feat(sdk): Add `Client:cross_process_store_locks_holder_name()` #4224

Hywan · 2024-11-06T14:38:07Z

We use cross-process store locks in 2 places now: in crypto, and in the event cache. The idea of this PR is to create a common place to define the cross-process store locks holder name.

This PR is twofold. First it creates Client::cross_process_store_locks_holder_name(), and second it uses this value for the EventCacheStoreLock.
~~This PR does not modify the crypto store lock for the moment. I don't know if it has to use the default/common holder name for the moment or not (ping @poljar, @pixlwave).~~ This PR also updates NotificationClient to automatically configure the client to use a new holder name, which is used by EncryptionSyncService to lock the crypto store accordingly.

jmartinesp

LGTM, but we should wait until someone can verify if we need the 'default' name.

crates/matrix-sdk/src/client/builder/mod.rs

codecov · 2024-11-06T15:08:50Z

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.90%. Comparing base (204e6e4) to head (4f95712).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-ui/src/sync_service.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4224      +/-   ##
==========================================
+ Coverage   84.87%   84.90%   +0.02%     
==========================================
  Files         274      274              
  Lines       29719    29716       -3     
==========================================
+ Hits        25225    25231       +6     
+ Misses       4494     4485       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr · 2024-11-06T15:15:18Z

This PR does not modify the crypto store lock for the moment. I don't know if it has to use the default/common holder name for the moment or not.

On iOS, the holder has to be different across the two different processes. This is the value that allows the app deciding who's holding onto the lock; so the main app would have to use one value, and any extension (notification service extension aka NSE, share extension) would have to use a different value uniquely identifying the service.

(The cross-process lock key has to be the same across all processes, though — otherwise processes will coordinate using different entries in the database.)

Hywan · 2024-11-06T15:44:41Z

@bnjbvr Yes, I'm aware of that, but I'm wondering if we should use the holder name from the Client or let the actual as it is. Looking at https://github.com/element-hq/element-x-ios/blob/10966ab620929517394ae1a4ceb9a458ff876c18/NSE/Sources/Other/NSEUserSession.swift#L31-L48, there are using NotificationClient directly, which uses Client::notification_client. I don't see where a new holder name is set for the cross-process lock of the crypto store.
Anyway, now on, the Client can setup a cross-process holder name, which can be passed to the NotificationClient and will be used everywhere, except by the crypto store for the moment.

In crates/matrix-sdk-crypto/src/store/mod.rs, we have Store::create_store_lock, which is used by OIDC and Encryption::enable_cross_process_store_lock. The latter is used only by EncryptionSyncService::new. And this one is used only by NotificationClient which pass its LOCK_ID constant. Sooooo. I think we need a little bit more refactoring on this one. Let's go.

This patch adds `ClientInner::cross_process_store_locks_holder_name` and its public method `Client::cross_process_store_locks_holder_name`. This patch also adds `ClientBuilder::cross_process_store_locks_holider_name` to configure this value.

…ere. See the Changelog Section to get the details. Changelog: `Client::cross_process_store_locks_holder_name` is used everywhere: - `StoreConfig::new()` now takes a `cross_process_store_locks_holder_name` argument. - `StoreConfig` no longer implements `Default`. - `BaseClient::new()` has been removed. - `BaseClient::clone_with_in_memory_state_store()` now takes a `cross_process_store_locks_holder_name` argument. - `BaseClient` no longer implements `Default`. - `EventCacheStoreLock::new()` no longer takes a `key` argument. - `BuilderStoreConfig` no longer has `cross_process_store_locks_holder_name` field for `Sqlite` and `IndexedDb`.

…t::cross_process_store_locks_holder_name`. 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.

…_refresh_crypto_lock`. This patch simplifies a little the `ClientBuilder` API: * `enable_cross_process_refresh_lock` is removed * `enable_oidc_refresh_crypto_lock` + `set_session_delegate` must be used instead.

Hywan · 2024-11-11T10:45:53Z

PR is ready for another round of review.

Complement Crypto is failing, because I've changed the public FFI API. I can disable Complement Crypto quickly, I'm already working on a patch.

jmartinesp

The new changes LGTM, thanks.

bindings/matrix-sdk-ffi/src/client_builder.rs

pixlwave

Otherwise, it all appears reasonable to me and a nice tidy-up 👏

This patch continues to simplification of the `matrix_sdk_ffi::Client`. The constructor can receive a `enable_oidc_refresh_lock: bool` instead of `cross_process_refresh_lock_id: Option<String>`, which was a copy of `matrix_sdk::Client::cross_process_store_locks_holder_name`. Now there is a single boolean to indicate whether `Oidc::enable_cross_process_refresh_lock` should be called or not. If it has to be called, it is possible to re-use `matrix_sdk::Client::cross_process_store_locks_holder_name`. Once again, there is a single place to read this data, it's not copied over different semantics.

Hywan requested a review from a team as a code owner November 6, 2024 14:38

Hywan requested review from jmartinesp and removed request for a team November 6, 2024 14:38

Hywan mentioned this pull request Nov 6, 2024

feat: Implement cross-process lock for the EventCache #4192

Merged

Hywan force-pushed the feat-sdk-client-process-holder branch 2 times, most recently from 6ff7493 to b9bd58f Compare November 6, 2024 14:49

jmartinesp reviewed Nov 6, 2024

View reviewed changes

crates/matrix-sdk/src/client/builder/mod.rs Show resolved Hide resolved

Hywan force-pushed the feat-sdk-client-process-holder branch from b9bd58f to 8169ad1 Compare November 6, 2024 15:16

Hywan force-pushed the feat-sdk-client-process-holder branch 4 times, most recently from c3e831a to f7c700b Compare November 11, 2024 10:18

Hywan added 5 commits November 11, 2024 11:18

feat(ffi): Add ClientBuilder::cross_process_store_locks_holder_name.

7eeff1b

fix(ffi): Replace enable_cross_process_refresh_lock by `enable_oidc…

d608457

…_refresh_crypto_lock`. This patch simplifies a little the `ClientBuilder` API: * `enable_cross_process_refresh_lock` is removed * `enable_oidc_refresh_crypto_lock` + `set_session_delegate` must be used instead.

Hywan force-pushed the feat-sdk-client-process-holder branch from f7c700b to 71b7f7f Compare November 11, 2024 10:23

Hywan requested review from jmartinesp and pixlwave November 11, 2024 10:46

jmartinesp approved these changes Nov 11, 2024

View reviewed changes

pixlwave reviewed Nov 11, 2024

View reviewed changes

bindings/matrix-sdk-ffi/src/client_builder.rs Outdated Show resolved Hide resolved

pixlwave approved these changes Nov 11, 2024

View reviewed changes

Hywan added 2 commits November 11, 2024 13:10

test(ci): Disable Complement Crypto for a short period of time.

4f95712

Hywan force-pushed the feat-sdk-client-process-holder branch from 71b7f7f to 4f95712 Compare November 11, 2024 12:12

Hywan merged commit 403be3d into matrix-org:main Nov 11, 2024
39 checks passed

This was referenced Nov 11, 2024

fix(rust): Migrate to the new API matrix-org/complement-crypto#153

Merged

test(ci): Re-enable Complement Crypto #4249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk): Add `Client:cross_process_store_locks_holder_name()` #4224

feat(sdk): Add `Client:cross_process_store_locks_holder_name()` #4224

Hywan commented Nov 6, 2024 •

edited

Loading

jmartinesp left a comment

codecov bot commented Nov 6, 2024 •

edited

Loading

bnjbvr commented Nov 6, 2024

Hywan commented Nov 6, 2024 •

edited

Loading

Hywan commented Nov 11, 2024

jmartinesp left a comment

pixlwave left a comment

feat(sdk): Add Client:cross_process_store_locks_holder_name() #4224

feat(sdk): Add Client:cross_process_store_locks_holder_name() #4224

Conversation

Hywan commented Nov 6, 2024 • edited Loading

jmartinesp left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

bnjbvr commented Nov 6, 2024

Hywan commented Nov 6, 2024 • edited Loading

Hywan commented Nov 11, 2024

jmartinesp left a comment

Choose a reason for hiding this comment

pixlwave left a comment

Choose a reason for hiding this comment

feat(sdk): Add `Client:cross_process_store_locks_holder_name()` #4224

feat(sdk): Add `Client:cross_process_store_locks_holder_name()` #4224

Hywan commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

Hywan commented Nov 6, 2024 •

edited

Loading