matrix-rust-sdk icon indicating copy to clipboard operation
matrix-rust-sdk copied to clipboard

Locks in the FFI layer are confusing

Open kegsay opened this issue 2 months ago • 1 comments

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

kegsay avatar Apr 16 '24 13:04 kegsay