fix: Don't change ConfiguredAddr when adding a transport #6804
Conversation
Doesn't matter yet because it's not possible yet to have multiple transports. But I wanted to fix this bug already so that I'm not suprised by it later.
| .sql | ||
| .set_raw_config(Config::ConfiguredAddr.as_ref(), Some(&addr)) | ||
| .await?; | ||
| if configured_addr.is_none() { |
There was a problem hiding this comment.
This can race with itself, e.g. if two set_config_ex() calls are done in parallel for an unconfigured context. While this is unlikely, i'd suggest to add some try_set_raw_config() which does INSERT OR IGNORE. Extra locking it's better to avoid. Orphaned transports entry shouldn't be a problem i think.
There was a problem hiding this comment.
The problem with this approach is that ConfiguredProvider should be set iff there was no ConfiguredAddr. While that would be possible to solve with some extra logic, I don't think the complexity (and by that, possibility of new bugs, and of confusing us in the future) is worth it. Many functions are not supposed to be called concurrently already (e.g. receive_imf()), and I don't see any reason why anyone would call save_to_transports_table() multiple times in parallel.
There was a problem hiding this comment.
The problem with this approach is that ConfiguredProvider should be set iff there was no ConfiguredAddr.
Not a problem, try_set_raw_config() could return bool (whether the insertion happened).
save_to_transports_table() won't be called in parallel directly of course because it's an internal function, but set_config() can be. Both calls would succeed then which doesn't look correct.
While there's no such problem for other config keys, maybe just document that set_config() mustn't be called in parallel for the same key? Then save_to_transports_table() wouldn't violate anything.
EDIT: But overall doing INSERT OR REPLACE for a config key which isn't supposed to change is bug-prone and will shoot sooner or later. But if this code is a temporary solution while multi-transport isn't fully supported, that's fine.
There was a problem hiding this comment.
ConfiguredProvider needs to go away eventually anyway. Then worst issue of the race condition is that another transport is set as default.
If race condition was a real problem here, then I'd start a transaction and do all the checks and replacement within it with raw config table changes. But in this case it is a config that is only changed in a response to UI actions or during initial bot setup.
Before this PR, ConfiguredAddr (which will be used to store the primary transport) would have been changed when adding a new transport. Doesn't matter yet because it's not possible yet to have multiple transports. But I wanted to fix this bug already so that I'm not suprised by it later.