Fix lost Waker instances with async stdio streams
#8782
Merged
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
This commit fixes a bug in the `Subscribe` trait implementation for
`AsyncStd{in,out}Stream` structures in the `wasmtime-wasi` crate.
Previously these implementations would create a future for the duration
of a single `poll` but then the future was dropped which could lead to
lost wakeups as the waker is gone after the future is dropped. The fix
was to use a `tokio::sync::Mutex` here instead of a `std::sync::Mutex`
and leave some comments about why contention isn't expected.
Closes bytecodealliance#8781
pchickey
reviewed
Jun 12, 2024
| }); | ||
|
|
||
| let mut buf = [0; 100]; | ||
| let n = read.read(&mut buf).await.unwrap(); |
There was a problem hiding this comment.
I'd be more comfortable with this test if the amount read was in excess of the buffer sizes in the io::duplex and the AsyncWriteStream. You could just lower those to 32 each.
pchickey
approved these changes
Jun 12, 2024
|
Why not just use |
|
Semantically these are all non-blocking functions, not blocking ones, so blocking on contention isn't appropriate for them. |
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.
This commit fixes a bug in the
Subscribetrait implementation forAsyncStd{in,out}Streamstructures in thewasmtime-wasicrate. Previously these implementations would create a future for the duration of a singlepollbut then the future was dropped which could lead to lost wakeups as the waker is gone after the future is dropped. The fix was to use atokio::sync::Mutexhere instead of astd::sync::Mutexand leave some comments about why contention isn't expected.Closes #8781