Bug 2054982 - Finer grained locking for RemoteSettingService#7474
Conversation
gruberb
left a comment
gruberb
left a comment
There was a problem hiding this comment.
I think we have to(please double check) combine creating the config and registering the client into one operation not to get out of sync. We can keep sync_client separate not to be blocking.
struct RemoteSettingsService {
storage_dir: Utf8PathBuf,
// Short-lived lock: configuration and client registration.
state: Mutex<RemoteSettingsServiceState>,
// Long-lived lock: held across the monitoring network request.
sync_client: Mutex<RemoteSettingsServiceSyncClient>,
telemetry: Mutex<RemoteSettingsTelemetryWrapper>,
}
struct RemoteSettingsServiceState {
config: RemoteSettingsServiceConfig,
clients: Vec<Weak<RemoteSettingsClient>>,
}| let config = self.clone_config(); | ||
| let client = Arc::new(RemoteSettingsClient::new( | ||
| inner.base_url.clone(), | ||
| inner.bucket_name.clone(), | ||
| config.base_url, | ||
| config.bucket_name, | ||
| collection_name.clone(), | ||
| inner.app_context.clone(), | ||
| config.app_context.clone(), | ||
| storage, | ||
| )); | ||
| inner.clients.push(Arc::downgrade(&client)); | ||
| self.push_client(Arc::downgrade(&client)); |
There was a problem hiding this comment.
Could make_client() race with update_config() here? clone_config() releases the config lock before this client is added to clients. If update_config() runs between these calls, active_clients() won’t include the new client, but the service config will be replaced. The client can then be registered with the old config and never receive the update.
Could config access and client registration/update share one short-lived mutex-protected state? sync_client could remain separate, so this would not reintroduce the original network-blocking issue.
There was a problem hiding this comment.
Oh yes it could indeed, nice catch.
There was a problem hiding this comment.
I followed your suggestion and put everything in one mutex and changed make_client to hold the mutex for the entire operation (which should be quick).
Split up `RemoteSettingServiceInner` into multiple structs, each with their own Mutex and add a bit of policy around locking them. Right now this is just enforced by putting all the methods that lock them in one impl block. The issue we're trying to avoid is having `RemoteSettingService::sync` block `RemoteSettingService::make_client`. Also, start using `viaduct::Client` which should be possible now since all applications are using the new backend.
bendk
force-pushed
the
bdk/push-qyzkwrspulzl
branch
from
d924c1f to
58dc98d
Compare
2 participants
Split up
RemoteSettingServiceInnerinto multiple structs, each with their own Mutex and add a bit of policy around locking them. Right now this is just enforced by putting all the methods that lock them in one impl block.The issue we're trying to avoid is having
RemoteSettingService::syncblockRemoteSettingService::make_client.Also, start using
viaduct::Clientwhich should be possible now since all applications are using the new backend.Pull Request checklist
[ci full]to the PR title.