Bug 2054982 - Finer grained locking for RemoteSettingService

[bendk]

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.

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

[gruberb]

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>>,
}

[gruberb]

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.

[bendk]

Oh yes it could indeed, nice catch.

[bendk]

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).

[gruberb]

I am not 100% sure if that's the case, but I think the race technically still exists? The update_config() still acquires the lock twice? Could we have a helper function like

fn update_config_and_clients(&self, config: RemoteSettingsServiceConfig) {
    let mut state = self.client_state.lock();

    let active_clients = state.active_clients();
    for client in active_clients {
        client.internal.update_config(
            config.base_url.clone(),
            config.bucket_name.clone(),
            config.app_context.clone(),
        );
    }

    state.config = config;
}