Skip to content
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

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

Merged

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Oct 30, 2024

This PR must be reviewed commit-by-commit.

It introduces the LockableEventCacheStore type, which wraps DynEventCacheStore, and that implements BackingStore so that it can be used inside the CrossProcessStoreLock.

It then introduces the EventCacheStoreLock type, which glue all pieces together: the CrossProcessStoreLock and the LockableEventCacheStore. It exposes a single method: lock(&'a self) -> Result<EventCacheStoreLockGuard<'a>, _>, which allows to take the lock. The guard derefs to DynEventCacheStore. The resulting API is straightforward to use according to me: client.event_cache_store().lock().await?, pretty Rust idiomatic.

Finally, the PR replaces all uses of DynEventCacheStore by EventCacheStoreLock. There is no more public DynEventCacheStore, expect via EventCacheStoreLock.

A follow-up patch must update matrix_sdk::event_cache to refresh the in-memory data of the event cache when the lock holder has changed. I believe it's nice to introduce that in another PR instead of increasing the number of patches in this one.


@Hywan Hywan force-pushed the feat-sdk-event-cache-cross-process-storage branch 2 times, most recently from c5dbe73 to 4e2bea3 Compare October 30, 2024 15:24
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

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

Project coverage is 84.89%. Comparing base (71abbeb) to head (c2e99b3).
Report is 40 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/client/builder/mod.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4192      +/-   ##
==========================================
- Coverage   84.89%   84.89%   -0.01%     
==========================================
  Files         272      272              
  Lines       29142    29208      +66     
==========================================
+ Hits        24739    24795      +56     
- Misses       4403     4413      +10     

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

Hywan added 2 commits October 30, 2024 16:58
This patch replaces a `&*` by a `.as_ref()` and a `.deref()`. The result
is the same but it's just simpler for newcomers to understand what
happens.
The idea is to avoid name conflicts when implementing other traits
that use the `Error` associated type.
@Hywan Hywan force-pushed the feat-sdk-event-cache-cross-process-storage branch 2 times, most recently from 666eba4 to 8aee160 Compare November 4, 2024 13:25
@Hywan Hywan force-pushed the feat-sdk-event-cache-cross-process-storage branch 4 times, most recently from 3d624d0 to f2ef774 Compare November 5, 2024 14:28
@Hywan Hywan force-pushed the feat-sdk-event-cache-cross-process-storage branch 2 times, most recently from 0c52043 to 4808481 Compare November 5, 2024 14:50
@Hywan Hywan force-pushed the feat-sdk-event-cache-cross-process-storage branch from 4808481 to c8f2321 Compare November 5, 2024 15:02
@Hywan Hywan marked this pull request as ready for review November 5, 2024 15:14
@Hywan Hywan requested review from a team as code owners November 5, 2024 15:14
@Hywan Hywan requested review from bnjbvr and richvdh and removed request for a team November 5, 2024 15:14
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! It's nice that we can reuse so much code for the cross-process locking 🥳

crates/matrix-sdk-base/src/client.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/store/mod.rs Show resolved Hide resolved
crates/matrix-sdk/src/client/builder/mod.rs Show resolved Hide resolved
Comment on lines +512 to +513
"default-key".to_owned(),
"matrix-sdk-base".to_owned(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values provided here are scary, and should be controllable by the callers, otherwise it's not possible to distinguish a process from another. The key could be a constant used everywhere in the codebase (maybe buried into a single place, and then passed around — or buried a single time when creating an EventCacheStoreLock from an event cache store impl), but the holder's value has to be provided by the embedder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to address that as a follow-up PR: I want to introduce a "process holder name" inside the client that can be used by all cross-process lock, otherwise things start to be really clumsy. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnjbvr and I have agreed to remove these values with a configuration in the Client. I'm already working on this as a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is, #4224

crates/matrix-sdk/src/client/builder/mod.rs Show resolved Hide resolved
@Hywan Hywan force-pushed the feat-sdk-event-cache-cross-process-storage branch from 47b4510 to 42d0dc4 Compare November 6, 2024 10:20
@Hywan Hywan requested a review from bnjbvr November 6, 2024 12:11
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@Hywan Hywan merged commit 0942dab into matrix-org:main Nov 6, 2024
40 checks passed
@Hywan Hywan mentioned this pull request Nov 28, 2024
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants