matrix-org / matrix-org/matrix-rust-sdk
Locks in the FFI layer are confusing
- Dominant language
- Rust
- Stars
- 2.3k
- Forks
- 500
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 106
Description
There's two distinct functions to control locks:
- `ClientBuilder.enableCrossProcessRefreshLock(processName, ...)`
- `SyncService.withCrossProcessLock(processName)`
Despite having similar names, they control different independent locks. The `ClientBuilder` variant controls the lock `store.create_store_lock("oidc_session_refresh_lock".to_owned(), lock_value.clone());` whereas the `SyncService` one controls `olm_machine.store().create_store_lock("cross_process_lock".to_owned(), lock_value);`.
Independent to this, creating a `NotificationClient` implicitly creates a lock on `cross_process_lock` with the value [LOCK_ID](https://github.com/matrix-org/matrix-rust-sdk/blob/f7329c71bbf518869a7d96e89114dfe297dffdab/crates/matrix-sdk-ui/src/notification_client.rs#L243) which is `"notifications"`.
This is all very confusing and very very footgunny, because if 2 independent processes create NotificationClient instances they will accidentally use the same lock value!
Proposed changes:
- Rename `enableCrossProcessRefreshLock` and `withCrossProcessLock` to better reflect which locks you are talking about e.g `enableOIDCCrossProcessLock` and `withEncryptionCrossProcessLock` would be much clearer IMO.
- Allow the lock value used in `NotificationClient` to be changed at the FFI layer, so the app can avoid making bad choices.
- Provide guidance and consistency in the terminology around the "lock value". One calls it an `appIdentifier` whereas the other calls it `processId`.
Contributor guide
Research direction
Start by comparing the FFI functions `ClientBuilder.enableCrossProcessRefreshLock` and `SyncService.withCrossProcessLock` with the lock creation sites named in the issue. Inspect `crates/matrix-sdk-ui/src/notification_client.rs`, especially the `LOCK_ID` usage, and trace how the lock value reaches the FFI layer. Done means the lock APIs are clearly named, NotificationClient's value can be configured through FFI, and `appIdentifier` and `processId` terminology is consistent.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- api
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100