waker: Fix weak memory issue in wake_by_ref() #7622
Merged
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
github-actions
bot
added
R-loom-current-thread
Darksonn
reviewed
Sep 15, 2025
frostyplanet
force-pushed
the
fix_deadlock
branch
from
c8bf72a to
186a289
Compare
|
Is it ok to use |
|
No. This is why I said the hardware is not relevant. Even on x86, compiler optimizations are allowed to have the same effect as what you saw on ARM. We need a release op no matter what. |
Darksonn
added
A-tokio
load(Acquire) might return old value. Previously, when reading waker state got is_notified(), it would return None directly, which could lead to a deadlock in race condition. To prevent this, use an additional CAS with the same value, to ensure we have a Release store pairing with Acquire load in transition_to_running(). tokio-rs#7589
frostyplanet
force-pushed
the
fix_deadlock
branch
from
186a289 to
6eb7cfa
Compare
|
The comment has been fixed |
This was referenced Sep 19, 2025
Merged
Closed
Merged
ADD-SP
pushed a commit
that referenced
this pull request
Oct 14, 2025
(cherry picked from commit 67869be)
Merged
1 task
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.
Motivation
When running with Miri, or dated arch like POWER, load(Acquire) might return old value.
But in the branch "COMPLETE || NOTIFIED" which returns None directly, might lead to a deadlock in a race condition.
Example code of deadlock report under Miri can be found in this issue: #7589
Solution
In this PR, use additional CAS with the same value to ensure a happen-before relationship. It will not be necessary on x86, but it's the smallest change， chances that waking the waker concurrently or waking the waker after it's complete might be rare.
An alternative solution might be changing the load to SeqCst, which will prevent Miri from reading old value,
but I am not sure what will happen when fetch_update_action() go to the branch a second time.