Skip to content
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

net: add clear_ready() to TcpStream, UdpSocket and Unix sockets #3716

Closed
wants to merge 1 commit into from

Conversation

eaufavor
Copy link

This change introduces clear_ready() similar to that in AsyncFd. This
change allows I/O operations outside tokio to work with ready ready().

Motivation

#2968 (and a few followups) means to help support custom async I/O operations (#3000). However because only tokio first party I/O operations clear the ready states, as a result, readiness is not reset correctly when pairing ready() with I/O operations external to tokio.

Solution

This change introduces clear_ready() similar to that in AsyncFd to help support external I/O operations.

This change introduces `clear_ready()` similar to that in AsyncFd. This
change allows I/O operations outside tokio to work with ready `ready()`.
@carllerche
Copy link
Member

Thanks for the PR. You bring up an interesting hole in the API. I worry that just adding clear_ready() may not be sufficient. The ready / clear API on AsncFd is the way it is to avoid race conditions. Consider:

  • Await read readiness
  • Try read operation, get WouldBlock
  • A new read event is received by the reactor
  • clear_ready() is called

In this case, clear_ready() will erase the new read event, resulting in the resource hanging.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Apr 20, 2021
@eaufavor
Copy link
Author

eaufavor commented Apr 20, 2021

Good point.
How about exposing the try_io(https://github.com/tokio-rs/tokio/blob/master/tokio/src/io/driver/registration.rs#L188) API instead?
Like

...
socket.readable().await;
socket.try_io(Interest::READABLE, ||{/* 3rd party API */})
...

That guarantee the 3rd party APIs are treated the same as first party try_* APIs without the issue you mentioned.

@carllerche
Copy link
Member

try_io is definitely one strategy. I'd be interested to hear if @bdonlan has any thoughts?

@bdonlan
Copy link
Contributor

bdonlan commented Apr 21, 2021

Using something like the ReadyGuard is another option. Essentially, before you perform the operation, take a token that contains the event sequence number, and on completion of the event clear the ready state only if a new event was not generated in the interim. This might work better with some styles of async operation, where the operation might complete asynchronously from the original future.

@carllerche
Copy link
Member

@bdonlan one issue is that we don't return ReadyGuard from the misc ready fns today 😬

@eaufavor
Copy link
Author

I'm close this PR for now given the comments and clearly there are better approaches.
I will give try_io a shot later.

@eaufavor eaufavor closed this Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants