-
Notifications
You must be signed in to change notification settings - Fork 305
feat: implement a generation counter for the CryptoStore lock #2155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
913c39a
to
801bde0
Compare
801bde0
to
d837280
Compare
d837280
to
f85f56e
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2155 +/- ##
==========================================
+ Coverage 76.59% 76.73% +0.13%
==========================================
Files 164 164
Lines 17594 17637 +43
==========================================
+ Hits 13477 13533 +56
+ Misses 4117 4104 -13
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A real-world test run would be nice before we merge though.
/// DB over time. Observing a different value than the one read in | ||
/// memory, when reading from the store indicates that somebody else has | ||
/// written into the database under our feet. | ||
pub(crate) crypto_store_generation: Arc<Mutex<Option<u64>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to use a SequenceNumber
for this?
matrix-rust-sdk/crates/matrix-sdk-crypto/src/store/caches.rs
Lines 172 to 182 in 06600ac
/// A numeric type that can represent an infinite ordered sequence. | |
/// | |
/// It uses wrapping arithmetic to make sure we never run out of numbers. (2**64 | |
/// should be enough for anyone, but it's easy enough just to make it wrap.) | |
// | |
/// Internally it uses a *signed* counter so that we can compare values via a | |
/// subtraction. For example, suppose we've just overflowed from i64::MAX to | |
/// i64::MIN. (i64::MAX.wrapping_sub(i64::MIN)) is -1, which tells us that | |
/// i64::MAX comes before i64::MIN in the sequence. | |
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] | |
pub(crate) struct SequenceNumber(i64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could use it, and it does impl PartialEq
/Eq
, but I'm really just using a single wrapping_add
here 😁
f85f56e
to
79f68ed
Compare
02cd58e
to
409b757
Compare
This implements a generation counter for the CryptoStore lock, aiming at solving the following problem. Consider ElementX iOS:
Then it's possible that the crypto store caches in the main app are now stale, but the main app won't realize, because trying to acquire the lock may succeed on the first attempt.
The solution is to introduce a "generation" counter, maintained in both a process embedding the SDK, and the store. It is updated any time we acquire a lock and we observed a different value in the store.
Now observe what happens with the above scenario, annotated with generations:
(When there's no mismatch, there's nothing to do, of course.)
Because of this, we don't need explicit cache reloading when the app goes back into the foreground, so it's slightly easier to use for ElementX embedders. And, it's more precise too as it won't do spurious cache reloads (in a scenario like main app goes to background / main app goes to foreground, with no NSE process involved).