feat: implement time lease based locks for the `CryptoStore` #2140

bnjbvr · 2023-06-23T10:49:59Z

This implements a new time lease based lock for the CryptoStore, that doesn't require explicit unlocking, so that's more robust in the context of #1928, where any process may die because the device is running out of battery, or unexpected flows cause a lock to not be released properly in one or the other process.

//! This is a per-process lock that may be used only for very specific use
//! cases, where multiple processes might concurrently write to the same
//! database at the same time; this would invalidate crypto store caches, so
//! that should be done mindfully. Such a lock can be acquired multiple times by
//! the same process, and it remains active as long as there's at least one user
//! in a given process.
//!
//! The lock is implemented using time-based leases to values inserted in a
//! crypto store. The store maintains the lock identifier (key), who's the
//! current holder (value), and an expiration timestamp on the side; see also
//! `CryptoStore::try_take_leased_lock` for more details.
//!
//! The lock is initially acquired for a certain period of time (namely, the
//! duration of a lease, aka `LEASE_DURATION_MS`), and then a "heartbeat" task
//! renews the lease to extend its duration, every so often (namely, every
//! `EXTEND_LEASE_EVERY_MS`). Since the tokio scheduler might be busy, the
//! extension request should happen way more frequently than the duration of a
//! lease, in case a deadline is missed. The current values have been chosen to
//! reflect that, with a ratio of 1:10 as of 2023-06-23.
//!
//! Releasing the lock happens naturally, by not renewing a lease. It happens
//! automatically after the duration of the last lease, at most.

Notes

This is not implemented on indexeddb, because time in wasm is slightly more complicated (requires a host API). Maybe we already have that, but I haven't bothered since I'm not sure that wasm may need the lock in the first place (maybe for a Web app using the SDK, running from multiple tabs?).
I've kept the former implementation for insert_custom_value_if_missing/remove_custom_value at the moment, but we could remove them if this lock proves to be more robust.

codecov · 2023-06-23T12:27:03Z

Codecov Report

Patch coverage: 76.50% and project coverage change: +0.22 🎉

Comparison is base (06600ac) 76.35% compared to head (21d2f0b) 76.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2140      +/-   ##
==========================================
+ Coverage   76.35%   76.57%   +0.22%     
==========================================
  Files         163      164       +1     
  Lines       17516    17565      +49     
==========================================
+ Hits        13374    13451      +77     
+ Misses       4142     4114      -28

Impacted Files	Coverage Δ
crates/matrix-sdk-crypto/src/store/mod.rs	`74.89% <0.00%> (+0.50%)`	⬆️
crates/matrix-sdk/src/room/joined/mod.rs	`63.41% <0.00%> (+0.91%)`	⬆️
crates/matrix-sdk/src/sliding_sync/list/mod.rs	`90.52% <ø> (-0.05%)`	⬇️
...rix-sdk/src/sliding_sync/list/request_generator.rs	`98.24% <ø> (ø)`
crates/matrix-sdk-ui/src/timeline/to_device.rs	`16.00% <16.00%> (ø)`
crates/matrix-sdk-common/src/ring_buffer.rs	`92.00% <50.00%> (-8.00%)`	⬇️
crates/matrix-sdk/src/encryption/mod.rs	`41.56% <50.00%> (-2.44%)`	⬇️
crates/matrix-sdk-ui/src/encryption_sync/mod.rs	`64.81% <56.25%> (-6.02%)`	⬇️
crates/matrix-sdk-crypto/src/store/locks.rs	`98.30% <97.56%> (+51.53%)`	⬆️
...s/matrix-sdk-crypto/src/store/integration_tests.rs	`100.00% <100.00%> (ø)`
... and 8 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ara4n · 2023-06-24T11:50:49Z

why do we need time leased locks? when i tested this with file based posix locks, you could acquire the lock no matter how dirtily the process holding the lock got knifed.

poljar · 2023-06-26T08:23:45Z

why do we need time leased locks? when i tested this with file based posix locks, you could acquire the lock no matter how dirtily the process holding the lock got knifed.

There isn't a great and obvious choice for a flock based crate and this was easier to implement in a platform-independent way in a timely manner. We certainly need to revise some parts of this as we go along.

jplatte

This is just to note that I've started review. I haven't gotten very far yet, but will continue tomorrow.

crates/matrix-sdk-crypto/src/store/locks.rs

Hywan

Overall it looks good to me :-).

Hywan · 2023-06-29T06:49:22Z

crates/matrix-sdk-crypto/src/store/locks.rs

@@ -68,6 +106,16 @@ pub struct CryptoStoreLock {
 }

 impl CryptoStoreLock {
+    /// Amount of time a lease of the lock should last, in milliseconds.
+    pub const LEASE_DURATION_MS: u32 = 2000;


Could be 1000, or even 800, so that it cannot be really perceived by the app end-user?

Fwiw: we chatted about it, and we'll try renew-every 50ms / lease-for 500ms: still a 1:10 ratio, and the lease should be closer to non-perceivable by the user (ideally we'd go under 300ms, but that seems more dangerous with respect to the scheduler being super busy).

crates/matrix-sdk-crypto/src/store/locks.rs

Hywan · 2023-06-29T06:53:39Z

crates/matrix-sdk-crypto/src/store/locks.rs

+            // Clone data to be owned by the task.
+            let this = self.clone();
+
+            matrix_sdk_common::executor::spawn(async move {


Are we sure we don't want to keep a handle to this spawned task, so that we can abort it in case of emergency?

Yes, it's also useful to make sure we don't spawn it multiple times 👍 Added some code to handle that.

poljar

I think this looks good. It would be nice if people on the EX side would test this but I guess that's what the nightly is there for.

From my point of view, we can tweak the timeouts if they end up being too long later on.

crates/matrix-sdk-crypto/src/store/locks.rs

poljar · 2023-06-29T08:56:08Z

crates/matrix-sdk/src/encryption/mod.rs

+            match lock.try_lock_once().await? {
+                Some(guard) => Ok(Some(guard)),
+                None => {
+                    // We didn't get the lock on the first attempt, so that means that another


Are we certain that we don't need to reload when we did acquire the lock on the first attempt? Isn't there a race where we might get the lock just as it was released by the other process?

Indeed you're right! This is going to be more generally fixed by #2155 :-)

Signed-off-by: Benjamin Bouvier <public@benj.me>

See top comment in matrix-sdk-crypto/src/store/locks.rs

Signed-off-by: Benjamin Bouvier <public@benj.me>

… there Signed-off-by: Benjamin Bouvier <public@benj.me>

… failed Signed-off-by: Benjamin Bouvier <public@benj.me>

Signed-off-by: Benjamin Bouvier <public@benj.me>

wat (also don't use `is_terminated` since it doesn't exist on wasm)

Hywan · 2023-06-29T12:03:51Z

crates/matrix-sdk-crypto/src/store/locks.rs

+            //   operation
+            // running in a transaction.
+
+            drop(renew_task.take());


Be careful, it doesn't abort the task!

Thanks, TIL. I've added an abort() that's guarded by #[cfg(not(target_arch = "wasm32"))]; this code wouldn't work on wasm32 anyways because it's using tokio::sleep.

Velin92 · 2023-06-29T12:30:03Z

Tested and this thing is bulletproof, tested on scenarios where both the NSE and Main App did not stop the sync and did not release the lock, and everything worked as expected.
@bnjbvr

bnjbvr requested a review from a team as a code owner June 23, 2023 10:50

bnjbvr requested review from jplatte and removed request for a team June 23, 2023 10:50

bnjbvr force-pushed the lease-locks branch 4 times, most recently from 9d6b67f to 5acbac6 Compare June 23, 2023 17:03

bnjbvr force-pushed the lease-locks branch from 082a32f to fb9bad0 Compare June 26, 2023 09:21

bnjbvr requested a review from poljar June 27, 2023 12:35

This was referenced Jun 27, 2023

room list previews and ordering can get minutes behind element-hq/element-x-ios#1173

Closed

Sliding sync: The Two Sync Loops #1928

Closed

jplatte reviewed Jun 27, 2023

View reviewed changes

crates/matrix-sdk-crypto/src/store/locks.rs Outdated Show resolved Hide resolved

crates/matrix-sdk-crypto/src/store/locks.rs Outdated Show resolved Hide resolved

Hywan approved these changes Jun 29, 2023

View reviewed changes

poljar approved these changes Jun 29, 2023

View reviewed changes

bnjbvr added 10 commits June 29, 2023 12:10

feat: implement a time lease based lock for the crypto store

54264e1

Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: switch the crypto-store lock a time-leased based one

a5470f1

See top comment in matrix-sdk-crypto/src/store/locks.rs

chore: fix CI, don't use unixepoch in sqlite and do time math in rust

40719d2

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: dummy implementation in indexeddb, don't run lease locks tests…

0b70682

… there Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: in NSE, wait the duration of a lease if first attempt to unlock…

f140a9e

… failed Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: immediately release the lock when there are no more holders

058d2a3

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: clippy

17b4919

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: add comment about atomic sanity

31ae38f

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: increase sleeps in timeline queue tests?

31ce42a

Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: lower lease and renew durations

b482365

bnjbvr force-pushed the lease-locks branch from fb9bad0 to 2f5e55c Compare June 29, 2023 10:25

feat: keep track of the extend-lease task

5e2f2d6

bnjbvr force-pushed the lease-locks branch from 2f5e55c to 5e2f2d6 Compare June 29, 2023 10:30

bnjbvr added 2 commits June 29, 2023 12:58

Merge branch 'main' into lease-locks

1ee318b

fix: increment num_holders when acquiring the lock for the first time

32e52ac

wat (also don't use `is_terminated` since it doesn't exist on wasm)

Hywan reviewed Jun 29, 2023

View reviewed changes

chore: reduce indent + abort prev renew task on non-wasm + add logs

21d2f0b

bnjbvr enabled auto-merge (squash) June 29, 2023 12:51

bnjbvr merged commit 73ad518 into matrix-org:main Jun 29, 2023

bnjbvr deleted the lease-locks branch June 29, 2023 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement time lease based locks for the `CryptoStore` #2140

feat: implement time lease based locks for the `CryptoStore` #2140

bnjbvr commented Jun 23, 2023

codecov bot commented Jun 23, 2023 •

edited

Loading

ara4n commented Jun 24, 2023

poljar commented Jun 26, 2023

jplatte left a comment

Hywan left a comment

Hywan Jun 29, 2023

bnjbvr Jun 29, 2023

Hywan Jun 29, 2023

bnjbvr Jun 29, 2023

poljar left a comment

poljar Jun 29, 2023

bnjbvr Jun 29, 2023

Hywan Jun 29, 2023

bnjbvr Jun 29, 2023

Velin92 commented Jun 29, 2023

feat: implement time lease based locks for the CryptoStore #2140

feat: implement time lease based locks for the CryptoStore #2140

Conversation

bnjbvr commented Jun 23, 2023

Notes

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

ara4n commented Jun 24, 2023

poljar commented Jun 26, 2023

jplatte left a comment

Choose a reason for hiding this comment

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Velin92 commented Jun 29, 2023

feat: implement time lease based locks for the `CryptoStore` #2140

feat: implement time lease based locks for the `CryptoStore` #2140

codecov bot commented Jun 23, 2023 •

edited

Loading