[cherrypick] Fix possible (but probably rare) race condition (#19951) #19966
Conversation
This file contains hidden or 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
@mystenmark do we also need this in |
ebmifa
approved these changes
Oct 22, 2024
Fix crashes in execution_driver due to inability to execute transactions. -------- We must hold the lock for the object entry while inserting to the `object_by_id_cache`. Otherwise, a surprising bug can occur: 1. A thread executing TX1 can write object (O,1) to the dirty set and then pause. 2. TX2, which reads (O,1) can begin executing, because TransactionManager immediately schedules transactions if their inputs are available. It does not matter that TX1 hasn't finished executing yet. 3. TX2 can write (O,2) to both the dirty set and the object_by_id_cache. 4. The thread executing TX1 can resume and write (O,1) to the object_by_id_cache. Now, any subsequent attempt to get the latest version of O will return (O,1) instead of (O,2). This seems very unlikely, but it may be more likely under the following circumstances: - While a thread is unlikely to pause for so long, moka cache uses optimistic lock-free algorithms that have retry loops. Possibly, under high contention, this code might spin for a surprisingly long time. - Additionally, many concurrent re-executions of the same tx could happen due to the tx finalizer, plus checkpoint executor, consensus, and RPCs from fullnodes. Unfortunately I have not been able to reproduce this bug, so we cannot be sure that this fixes the crashes we've seen. But this is certainly a possible bug.
mystenmark
force-pushed
the
mlogan-cp-crash-fix
branch
from
f546d90 to
331ed72
Compare
yes, opened #19967 |
mystenmark
merged commit d640e32
into
releases/sui-v1.35.0-release
44 of 49 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fix crashes in execution_driver due to inability to execute
transactions.
We must hold the lock for the object entry while inserting to the
object_by_id_cache. Otherwise, a surprising bug can occur:then pause.
TransactionManager immediately
schedules transactions if their inputs are available. It does not matter
that TX1
hasn't finished executing yet.
object_by_id_cache.
Now, any subsequent attempt to get the latest version of O will return
(O,1) instead of
(O,2).
This seems very unlikely, but it may be more likely under the following
circumstances:
optimistic
lock-free algorithms that have retry loops. Possibly, under high
contention, this
code might spin for a surprisingly long time.
happen due to
the tx finalizer, plus checkpoint executor, consensus, and RPCs from
fullnodes.
Unfortunately I have not been able to reproduce this bug, so we cannot
be sure that
this fixes the crashes we've seen. But this is certainly a possible bug.
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.