From 563c3aae3145a1839caeecb4b9d3e629f875d7a5 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 6 Nov 2024 16:55:04 +0100 Subject: [PATCH] feat(ui): `EncryptionSyncService` and `Notification` are using `Client::cross_process_store_locks_holder_name`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch removes the `process_id` argument from `EncryptionSyncService::new()` and replaces it by `Client::cross_process_store_locks_holder_name`. The “process ID” is set when the `Client` is converted into another `Client` tailore for notification in `NotificationClient` with `Client::notification_client` which now has a new `cross_process_store_locks_holder_name` argument. --- bindings/matrix-sdk-ffi/src/sync_service.rs | 4 ++-- .../src/encryption_sync_service.rs | 12 +++++++----- crates/matrix-sdk-ui/src/notification_client.rs | 4 ++-- crates/matrix-sdk-ui/src/sync_service.rs | 17 ++++------------- .../integration/encryption_sync_service.rs | 17 ++++++----------- crates/matrix-sdk/src/client/mod.rs | 16 +++++++++++----- crates/matrix-sdk/src/encryption/mod.rs | 2 ++ .../src/helpers.rs | 12 ++++++++++++ .../src/tests/nse.rs | 14 +++++++++----- 9 files changed, 55 insertions(+), 43 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/sync_service.rs b/bindings/matrix-sdk-ffi/src/sync_service.rs index dd1e6b89d..52179678e 100644 --- a/bindings/matrix-sdk-ffi/src/sync_service.rs +++ b/bindings/matrix-sdk-ffi/src/sync_service.rs @@ -112,9 +112,9 @@ impl SyncServiceBuilder { #[matrix_sdk_ffi_macros::export] impl SyncServiceBuilder { - pub fn with_cross_process_lock(self: Arc, app_identifier: Option) -> Arc { + pub fn with_cross_process_lock(self: Arc) -> Arc { let this = unwrap_or_clone_arc(self); - let builder = this.builder.with_cross_process_lock(app_identifier); + let builder = this.builder.with_cross_process_lock(); Arc::new(Self { client: this.client, builder, utd_hook: this.utd_hook }) } diff --git a/crates/matrix-sdk-ui/src/encryption_sync_service.rs b/crates/matrix-sdk-ui/src/encryption_sync_service.rs index 66da6e890..67148fc7f 100644 --- a/crates/matrix-sdk-ui/src/encryption_sync_service.rs +++ b/crates/matrix-sdk-ui/src/encryption_sync_service.rs @@ -88,11 +88,7 @@ impl EncryptionSyncService { /// Creates a new instance of a `EncryptionSyncService`. /// /// This will create and manage an instance of [`matrix_sdk::SlidingSync`]. - /// The `process_id` is used as the identifier of that instance, as such - /// make sure to not reuse a name used by another process, at the risk - /// of causing problems. pub async fn new( - process_id: String, client: Client, poll_and_network_timeouts: Option<(Duration, Duration)>, with_locking: WithLocking, @@ -119,7 +115,13 @@ impl EncryptionSyncService { if with_locking { // Gently try to enable the cross-process lock on behalf of the user. - match client.encryption().enable_cross_process_store_lock(process_id).await { + match client + .encryption() + .enable_cross_process_store_lock( + client.cross_process_store_locks_holder_name().to_owned(), + ) + .await + { Ok(()) | Err(matrix_sdk::Error::BadCryptoStoreState) => { // Ignore; we've already set the crypto store lock to // something, and that's sufficient as diff --git a/crates/matrix-sdk-ui/src/notification_client.rs b/crates/matrix-sdk-ui/src/notification_client.rs index 0c80298f0..48396c577 100644 --- a/crates/matrix-sdk-ui/src/notification_client.rs +++ b/crates/matrix-sdk-ui/src/notification_client.rs @@ -111,7 +111,8 @@ impl NotificationClient { parent_client: Client, process_setup: NotificationProcessSetup, ) -> Result { - let client = parent_client.notification_client().await?; + let client = parent_client.notification_client(Self::LOCK_ID.to_owned()).await?; + Ok(NotificationClient { client, parent_client, @@ -242,7 +243,6 @@ impl NotificationClient { }; let encryption_sync = EncryptionSyncService::new( - Self::LOCK_ID.to_owned(), self.client.clone(), Some((Duration::from_secs(3), Duration::from_secs(4))), with_locking, diff --git a/crates/matrix-sdk-ui/src/sync_service.rs b/crates/matrix-sdk-ui/src/sync_service.rs index 23c8166ca..7ff37bfbf 100644 --- a/crates/matrix-sdk-ui/src/sync_service.rs +++ b/crates/matrix-sdk-ui/src/sync_service.rs @@ -435,15 +435,11 @@ pub struct SyncServiceBuilder { /// Is the cross-process lock for the crypto store enabled? with_cross_process_lock: bool, - - /// Application identifier, used as the cross-process lock value, if - /// applicable. - identifier: String, } impl SyncServiceBuilder { fn new(client: Client) -> Self { - Self { client, with_cross_process_lock: false, identifier: "app".to_owned() } + Self { client, with_cross_process_lock: false } } /// Enables the cross-process lock, if the sync service is being built in a @@ -454,14 +450,10 @@ impl SyncServiceBuilder { /// external process attempting to decrypt notifications. In general, /// `with_cross_process_lock` should not be called. /// - /// An app identifier can be provided too, to identify the current process; - /// if it's not provided, a default value of "app" is used as the - /// application identifier. - pub fn with_cross_process_lock(mut self, app_identifier: Option) -> Self { + /// Be sure to have configured + /// [`Client::cross_process_store_locks_holder_name`] accordingly. + pub fn with_cross_process_lock(mut self) -> Self { self.with_cross_process_lock = true; - if let Some(app_identifier) = app_identifier { - self.identifier = app_identifier; - } self } @@ -477,7 +469,6 @@ impl SyncServiceBuilder { let encryption_sync = Arc::new( EncryptionSyncService::new( - self.identifier, self.client, None, WithLocking::from(self.with_cross_process_lock), diff --git a/crates/matrix-sdk-ui/tests/integration/encryption_sync_service.rs b/crates/matrix-sdk-ui/tests/integration/encryption_sync_service.rs index e97bd0b93..299dd3f84 100644 --- a/crates/matrix-sdk-ui/tests/integration/encryption_sync_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/encryption_sync_service.rs @@ -40,8 +40,7 @@ async fn test_smoke_encryption_sync_works() -> anyhow::Result<()> { let sync_permit = Arc::new(AsyncMutex::new(EncryptionSyncPermit::new_for_testing())); let sync_permit_guard = sync_permit.clone().lock_owned().await; - let encryption_sync = - EncryptionSyncService::new("tests".to_owned(), client, None, WithLocking::Yes).await?; + let encryption_sync = EncryptionSyncService::new(client, None, WithLocking::Yes).await?; let stream = encryption_sync.sync(sync_permit_guard); pin_mut!(stream); @@ -186,8 +185,7 @@ async fn test_encryption_sync_one_fixed_iteration() -> anyhow::Result<()> { let sync_permit = Arc::new(AsyncMutex::new(EncryptionSyncPermit::new_for_testing())); let sync_permit_guard = sync_permit.lock_owned().await; - let encryption_sync = - EncryptionSyncService::new("tests".to_owned(), client, None, WithLocking::Yes).await?; + let encryption_sync = EncryptionSyncService::new(client, None, WithLocking::Yes).await?; // Run all the iterations. encryption_sync.run_fixed_iterations(1, sync_permit_guard).await?; @@ -218,8 +216,7 @@ async fn test_encryption_sync_two_fixed_iterations() -> anyhow::Result<()> { let sync_permit = Arc::new(AsyncMutex::new(EncryptionSyncPermit::new_for_testing())); let sync_permit_guard = sync_permit.lock_owned().await; - let encryption_sync = - EncryptionSyncService::new("tests".to_owned(), client, None, WithLocking::Yes).await?; + let encryption_sync = EncryptionSyncService::new(client, None, WithLocking::Yes).await?; encryption_sync.run_fixed_iterations(2, sync_permit_guard).await?; @@ -254,8 +251,7 @@ async fn test_encryption_sync_always_reloads_todevice_token() -> anyhow::Result< let sync_permit = Arc::new(AsyncMutex::new(EncryptionSyncPermit::new_for_testing())); let sync_permit_guard = sync_permit.lock_owned().await; let encryption_sync = - EncryptionSyncService::new("tests".to_owned(), client.clone(), None, WithLocking::Yes) - .await?; + EncryptionSyncService::new(client.clone(), None, WithLocking::Yes).await?; let stream = encryption_sync.sync(sync_permit_guard); pin_mut!(stream); @@ -363,15 +359,14 @@ async fn test_notification_client_does_not_upload_duplicate_one_time_keys() -> a info!("Creating the notification client"); let notification_client = client - .notification_client() + .notification_client("tests".to_owned()) .await .expect("We should be able to build a notification client"); let sync_permit = Arc::new(AsyncMutex::new(EncryptionSyncPermit::new_for_testing())); let sync_permit_guard = sync_permit.lock_owned().await; let encryption_sync = - EncryptionSyncService::new("tests".to_owned(), client.clone(), None, WithLocking::Yes) - .await?; + EncryptionSyncService::new(client.clone(), None, WithLocking::Yes).await?; let stream = encryption_sync.sync(sync_permit_guard); pin_mut!(stream); diff --git a/crates/matrix-sdk/src/client/mod.rs b/crates/matrix-sdk/src/client/mod.rs index 2eac8c7d9..e8a6f98fb 100644 --- a/crates/matrix-sdk/src/client/mod.rs +++ b/crates/matrix-sdk/src/client/mod.rs @@ -2253,7 +2253,15 @@ impl Client { } /// Create a new specialized `Client` that can process notifications. - pub async fn notification_client(&self) -> Result { + /// + /// See [`CrossProcessStoreLock::new`] to learn more about + /// `cross_process_store_locks_holder_name`. + /// + /// [`CrossProcessStoreLock::new`]: matrix_sdk_common::store_locks::CrossProcessStoreLock::new + pub async fn notification_client( + &self, + cross_process_store_locks_holder_name: String, + ) -> Result { let client = Client { inner: ClientInner::new( self.inner.auth_ctx.clone(), @@ -2264,9 +2272,7 @@ impl Client { self.inner.http_client.clone(), self.inner .base_client - .clone_with_in_memory_state_store( - &self.inner.cross_process_store_locks_holder_name, - ) + .clone_with_in_memory_state_store(&cross_process_store_locks_holder_name) .await?, self.inner.server_capabilities.read().await.clone(), self.inner.respect_login_well_known, @@ -2274,7 +2280,7 @@ impl Client { self.inner.send_queue_data.clone(), #[cfg(feature = "e2e-encryption")] self.inner.e2ee.encryption_settings, - self.inner.cross_process_store_locks_holder_name.clone(), + cross_process_store_locks_holder_name, ) .await, }; diff --git a/crates/matrix-sdk/src/encryption/mod.rs b/crates/matrix-sdk/src/encryption/mod.rs index 0ac2d38dc..a22ebee2c 100644 --- a/crates/matrix-sdk/src/encryption/mod.rs +++ b/crates/matrix-sdk/src/encryption/mod.rs @@ -1458,6 +1458,8 @@ impl Encryption { /// caches. /// /// The provided `lock_value` must be a unique identifier for this process. + /// Check [`Client::cross_process_store_locks_holder_name`] to + /// get the global value. pub async fn enable_cross_process_store_lock(&self, lock_value: String) -> Result<(), Error> { // If the lock has already been created, don't recreate it from scratch. if let Some(prev_lock) = self.client.locks().cross_process_crypto_store_lock.get() { diff --git a/testing/matrix-sdk-integration-testing/src/helpers.rs b/testing/matrix-sdk-integration-testing/src/helpers.rs index 9b4ee6a30..18614c6ed 100644 --- a/testing/matrix-sdk-integration-testing/src/helpers.rs +++ b/testing/matrix-sdk-integration-testing/src/helpers.rs @@ -37,6 +37,7 @@ pub struct TestClientBuilder { use_sqlite_dir: Option, encryption_settings: EncryptionSettings, http_proxy: Option, + cross_process_store_locks_holder_name: Option, } impl TestClientBuilder { @@ -52,6 +53,7 @@ impl TestClientBuilder { use_sqlite_dir: None, encryption_settings: Default::default(), http_proxy: None, + cross_process_store_locks_holder_name: None, } } @@ -79,6 +81,11 @@ impl TestClientBuilder { self } + pub fn cross_process_store_locks_holder_name(mut self, holder_name: String) -> Self { + self.cross_process_store_locks_holder_name = Some(holder_name); + self + } + fn common_client_builder(&self) -> ClientBuilder { let homeserver_url = option_env!("HOMESERVER_URL").unwrap_or("http://localhost:8228").to_owned(); @@ -90,6 +97,11 @@ impl TestClientBuilder { .with_encryption_settings(self.encryption_settings) .request_config(RequestConfig::short_retry()); + if let Some(holder_name) = &self.cross_process_store_locks_holder_name { + client_builder = + client_builder.cross_process_store_locks_holder_name(holder_name.clone()); + } + if let Some(proxy) = &self.http_proxy { client_builder = client_builder.proxy(proxy); } diff --git a/testing/matrix-sdk-integration-testing/src/tests/nse.rs b/testing/matrix-sdk-integration-testing/src/tests/nse.rs index 2bbb1bd12..56655182b 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/nse.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/nse.rs @@ -125,14 +125,18 @@ impl ClientWrapper { /// Otherwise, a random path is used. /// /// The contained SyncService always has a cross-process lock. If - /// app_identifier is supplied, it is used to identify this client's - /// process. If not, the default name is used. + /// `cross_process_store_locks_holder_name` is supplied, it is used to + /// identify this client's process. If not, the default name is used. async fn new( username: &str, sqlite_dir: Option<&Path>, - app_identifier: Option, + cross_process_store_locks_holder_name: Option, ) -> Self { - let builder = TestClientBuilder::new(username); + let mut builder = TestClientBuilder::new(username); + + if let Some(holder_name) = cross_process_store_locks_holder_name { + builder = builder.cross_process_store_locks_holder_name(holder_name); + } let builder = if let Some(sqlite_dir) = sqlite_dir { builder.use_sqlite_dir(sqlite_dir) @@ -153,7 +157,7 @@ impl ClientWrapper { let client = SyncTokenAwareClient::new(inner_client.clone()); let sync_service = SyncService::builder(inner_client) - .with_cross_process_lock(app_identifier) + .with_cross_process_lock() .build() .await .expect("Failed to create sync service");