-
Notifications
You must be signed in to change notification settings - Fork 353
feat(common): Add cross-process lock dirtying #5672
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(common): Add cross-process lock dirtying #5672
Conversation
3c168ad to
976f768
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5672 +/- ##
=======================================
Coverage 88.54% 88.55%
=======================================
Files 361 361
Lines 101533 101629 +96
Branches 101533 101629 +96
=======================================
+ Hits 89904 89998 +94
+ Misses 7418 7415 -3
- Partials 4211 4216 +5 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5672 will not alter performanceComparing Summary
|
39b6f56 to
b29747d
Compare
891db14 to
985032c
Compare
33f45c4 to
960bc89
Compare
|
Must be merged after #5829. |
This patch adds `CrossProcessLockGeneration`. A lock generation is an integer incremented each time the lock is taken by another holder. If the generation changes, it means the lock is _dirtied_. This _dirtying_ aspect is going to be expanded in the next patches. This patch focuses on the introduction of this _generation_. The `CrossProcessLock::try_lock_once` method, and the `TryLock::try_lock` method, both returns a `Option<CrossProcessLockGeneration>` instead of a `bool`: `true` is replaced by `Some(_)`, `false` by `None`.
…cache stores. This patch adds `Lease::generation` support in the crypto, media and event cache stores. For the crypto store, we add the new `lease_locks` object store/table. Previously, `Lease` was stored in `core`, but without any prefix, it's easy to overwrite another records, it's dangerous. The sad thing is that it's hard to delete the existing leases in `core` because the keys aren't known. See the comment in the code explaining the tradeoff. For media and event cache stores, the already existing `leases` object store/table is cleared so that we can change the format of `Lease` easily.
960bc89 to
d444e68
Compare
This patch detects when the cross-process lock has been dirtied. A new `CrossProcessLockResult` enum is introduced to simplify the returned value of `try_lock_once` and `spin_lock`. It flattens the previous `Result<Option<_>>` by providing 3 variants: `Clean`, `Dirty` and `Unobtained`.
21f3014 to
d5c68c0
Compare
poljar
left a comment
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.
Alright, I think all of this makes sense to me and it looks good.
| UPDATE SET | ||
| holder = excluded.holder, | ||
| expiration = excluded.expiration, | ||
| generation = | ||
| CASE holder | ||
| WHEN excluded.holder THEN generation | ||
| ELSE generation + 1 | ||
| END | ||
| WHERE | ||
| holder = excluded.holder | ||
| OR expiration < ?4 | ||
| RETURNING generation |
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.
It feels a bit wrong to have this three times, but moving this to a common DB seems like it would cause other kind of trouble.
I don't have a better idea here 😅
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.
We could refactor that later to have a common implementation.
| impl CrossProcessLockResult { | ||
| /// Convert from [`CrossProcessLockResult`] to | ||
| /// [`Option<T>`] where `T` is [`CrossProcessLockGuard`]. | ||
| pub fn ok(self) -> Option<CrossProcessLockGuard> { | ||
| match self { | ||
| Self::Clean(guard) | Self::Dirty(guard) => Some(guard), | ||
| Self::Unobtained => None, | ||
| } | ||
| } | ||
|
|
||
| /// Return `true` if the lock has been obtained, `false` otherwise. | ||
| pub fn is_ok(&self) -> bool { | ||
| matches!(self, Self::Clean(_) | Self::Dirty(_)) | ||
| } | ||
| } |
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.
Something we mentioned in chat but for archeological reasons I'll repeat it here.
I don't think it's worth it to reimplement the Result type to avoid having Result<Result<T>>.
But I think we left the modification of the return type to another PR.
So nothing to do here.
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.
Thanks for the future us.
This set of patches updates the cross-process lock to introduce “dirtiness”. The lock is considered “dirty” when the lock is acquired from another holder:
To achieve so, a generation value is added in the stores. This generation is a
u64. Every time the lock is acquired from a previous holder, this generation is incremented by one. That's basically it. The first value for the generation is 0, which acts as a guard value.The
TryLock::try_lockmethod now returns anOption<CrossProcessLockGeneration>instead ofbool.The
CrossProcessLock::try_lock_oncemethod now returns aCrossProcessLockResultinstead of anOption<CrossProcessLockGuard>. It's a new type that looks like this:The
CrossProcessLock::spin_lockmethod doesn't change for the moment. Let's do that in another PR where we will use the new API to decide how to invalidate the various states here and there. I wanted to keep this PR as small as possible first.