[Cherry-Pick] Fix bug in object_by_id_cache (#20450) #20613
Merged
+215
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Suppose that a reader thread is trying to cache an object that it just
read, while a writer thread is trying to cache an object that it just
wrote. The writer thread definitionally has the latest version. The
reader thread may be out of date. While we previously took some care to
not replace a new version with an old version, this did not take into
account evictions, and so the following bug was possible:
READER WRITER
read object_by_id_cache (miss)
read dirty set (miss)
write to dirty
read db (old version)
write to cache (while holding dirty lock)
cache entry is evicted
write to cache
There is no way for the reader to tell that the value it is caching is
out of date, because the up to date entry is already gone from the
cache.
We fix this by requiring reader threads to obtain a ticket before they
read from the dirty set and/or db. Tickets are expired by writers. Then,
the above case looks like this:
READER WRITER
get ticket
read cache (miss)
read dirty (miss)
write dirty
read db (old version)
expire ticket
write cache (while holding dirty lock)
cache eviction
no write to cache (ticket expired)
Any interleaving of the above either results in the reader seeing a
recent version, or else having an expired ticket.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
|
@mystenmark, do we need this for |
ebmifa
approved these changes
Dec 12, 2024
arun-koshy
approved these changes
Dec 12, 2024
ebmifa
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
mystenmark
merged commit 44cbc21
into
releases/sui-v1.38.0-release
48 of 52 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suppose that a reader thread is trying to cache an object that it just
read, while a writer thread is trying to cache an object that it just
wrote. The writer thread definitionally has the latest version. The
reader thread may be out of date. While we previously took some care to
not replace a new version with an old version, this did not take into
account evictions, and so the following bug was possible:
write to cache (while holding dirty lock)
cache entry is evicted
write to cache
There is no way for the reader to tell that the value it is caching is
out of date, because the up to date entry is already gone from the
cache.
We fix this by requiring reader threads to obtain a ticket before they
read from the dirty set and/or db. Tickets are expired by writers. Then,
the above case looks like this:
write cache (while holding dirty lock)
cache eviction
no write to cache (ticket expired)
Any interleaving of the above either results in the reader seeing a
recent version, or else having an expired ticket.
Description
Describe the changes or additions included in this PR.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.