Repository navigation
fix(tokio,async_io): keep a waker armed after draining to WouldBlock - #35
Open
glamberson wants to merge 1 commit into
Open
glamberson wants to merge 1 commit into
glamberson wants to merge 1 commit into
Conversation
EiEventStream::poll_next drains the socket to WouldBlock, clears readiness, and returns Pending directly, in both the tokio and async-io backends. Neither poll_read_ready_mut nor Async::poll_readable stores a waker on the call that already found the descriptor ready: Per their own documentation, a Ready return only means an edge was delivered since the last Pending, and the waker is armed only on the call that actually finds nothing ready. So a poll that goes ready, drains, clears, and returns Pending directly leaves no waker registered for whatever arrives next. I hit this in a production RDP server using the tokio backend to drive an EIS handshake against Mutter's RemoteDesktop portal: The connection went to sleep with the compositor's reply already sitting unread on the socket, roughly one activation in fifty, and since the handshake runs on the accept task, every later client sat in the listen backlog until the process was restarted. The fix loops back to poll_read_ready_mut (poll_readable for async-io) after clear_ready() instead of returning Pending directly, so the call that ends up returning Pending is always the one that arms a waker. This is also the shape tokio's own internal Registration::poll_io already uses for the same read-drain-clear pattern (runtime/io/registration.rs), just not one EiEventStream reimplements by hand. Applied to both backends since they share the same shape.
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
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.
EiEventStream::poll_next drains the socket to WouldBlock, clears readiness,
and returns Pending directly, in both the tokio and async-io backends.
Neither poll_read_ready_mut nor Async::poll_readable stores a waker on the
call that already found the descriptor ready: per their own documentation, a
Ready return only means an edge was delivered since the last Pending, and the
waker is armed only on the call that actually finds nothing ready. So a poll
that goes ready, drains, clears, and returns Pending directly leaves no waker
registered for whatever arrives next.
I hit this in a production RDP server using the tokio backend to drive an EIS
handshake against Mutter's RemoteDesktop portal: the connection went to sleep
with the compositor's reply already sitting unread on the socket, roughly one
activation in fifty, and since the handshake runs on the accept task, every
later client sat in the listen backlog until the process was restarted.
The fix loops back to poll_read_ready_mut (poll_readable for async-io) after
clear_ready() instead of returning Pending directly, so the call that ends up
returning Pending is always the one that arms a waker. This is also the shape
tokio's own internal Registration::poll_io already uses for the same
read-drain-clear pattern (runtime/io/registration.rs), just not one
EiEventStream reimplements by hand. Applied to both backends since they share
the same shape.
fmt, clippy --all-features --all-targets -Dwarnings (and the non-stable
--all-features variant), cargo check, cargo build --all-features --examples,
and cargo doc --deny warnings (with and without --all-features) all pass.
There's no unit-test harness in the crate for this path; I could not get a
small standalone repro to reliably discriminate old vs fixed code (the race
needs the scheduling of a busier multi-task runtime, consistent with it only
firing about 2% of the time in practice), so I'm not including one. The
diagnosis rests on the documented poll contracts cited above plus the
production fix already deployed against the real symptom.