-
Notifications
You must be signed in to change notification settings - Fork 800
fix: peek reregister after would block #1895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Reregister is an expensive operation and I don't think that's the solution we want. |
How about handle peek event specially in
|
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
97f3dca
to
5f64c3f
Compare
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
the falling CI seems related to #1883 and not caused by this patch |
tests/tcp_stream.rs
Outdated
assert_eq!(stream2.write(&[0, 1, 2, 3]).unwrap(), 4); | ||
|
||
// this panic with no event on windows if not re-register after would block peek | ||
// becuase mio simulate edge-triggered behavior in `SockState::feed_event` and need reregister |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is outdated and can be removed.
Thanks @discord9 |
For what it is worth, this is the exact fix that I proposed 2 years ago. @Thomasdezeeuw did something fundamentally changed in core for your opinion to change on that? See #1756 |
I think we just misunderstood the problem space last time we looked at it. I don't think there is anything more to it. Sorry about that. |
CC #1755 tokio-rs/tokio#3789
as title, use a fix that on windows
peek
should reregister aftereither success orwould block, so that it's behavior would be the same as on linux(seepeek_readiness
peek_ok
andpeek_would_block
).This should fix windows peek problem, @Darksonn might want to take a look.
The problem it seems is that in
src\sys\windows\selector.rs
SockState::feed_event
mio have to simulate Edge-triggered behavior and clean readable interest after send a readable interest, but since peek is also a kind of special read, it also cancelled interest to readable event, hence cause the blocking peek bug on windows because interest is lost so blocking is foreveredited: found out that under linux with
cfg(mio_unsupported_force_poll_poll)
peek would also have similiar problem so just writedo_io
on all platform insteadAlternatives
only do
do_io
on#[cfg(any(windows, mio_unsupported_force_poll_poll))]
which also seem to pass all tests on all platform/cfg combination