Allow specifying custom interest for TcpStream #5796
Conversation
|
No automated tests have been added; I'm not sure where it would be appropriate to add them, particularly given that |
leftmostcat
force-pushed
the
tcp_interest
branch
from
9469ce5
to
14d341b
Compare
With the addition of Interest::PRIORITY, it is possible to ask `ready()` to wake on urgent data. However, because `PollEvented` is set up with only read/write interest, `ready()`'s future will never complete. Add support for specifying custom interest in the creation of `PollEvented` to allow `ready()` to correctly poll for urgent. Fixes: tokio-rs#5784
leftmostcat
force-pushed
the
tcp_interest
branch
from
14d341b
to
6d5a255
Compare
There was a problem hiding this comment.
I do think a test for this logic is required. I'd guess tests/tcp_stream or tests/tcp_socket is the natural location. You can just use a cfg(target_os = "linux") on the test, cfgs happen in a bunch of places already, also in the tests.
In 4a254e9 I even made a custom file for the test. The code was removed from #5566 to simplify that PR (and because I don't need that functionality), but it may provide inspiration now.
| @@ -158,13 +158,58 @@ impl TcpListener { | |||
| /// } | |||
| /// ``` | |||
| pub async fn accept(&self) -> io::Result<(TcpStream, SocketAddr)> { | |||
| self.accept_with_interest(Default::default()).await | |||
There was a problem hiding this comment.
not sure if this is a general practice, but I'd prefer Interest::default() here to make it clearer that we're passing an interest.
There was a problem hiding this comment.
this is also assuming that the Default instance is a good choice, which I'm not sure about.
| impl Default for Interest { | ||
| fn default() -> Self { | ||
| Interest::READABLE.add(Interest::WRITABLE) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
is this truly a reasonable default for Interest? I think Interest::READABLE | Interest::WRITABLE written out in full is always clearer.
|
I'm closing this as stale. There are some reviews by folkertdev that would be good to address. If you want to continue this work, feel free to reopen. |
|
@leftmostcat Hi, are you still actively working on this? If not, I would like to continue working on this. |
With the addition of Interest::PRIORITY, it is possible to ask
ready()to wake on urgent data. However, becausePollEventedis set up with only read/write interest,ready()'s future will never complete.Add support for specifying custom interest in the creation of
PollEventedto allowready()to correctly poll for urgent.Fixes: #5784
Motivation
It is not currently possible to poll for urgent data via
TcpStream.Interest::PRIORITYwas added inmio, support for it was added toAsyncFd, and it's possible to passInterest::PRIORITYtoTcpStream::ready(), but it it will never be resolved due to a lack of API for registering the interest forPollEvented.Solution
Provide API parallel to
AsyncFd'swith_interest()to API which returns aTcpStream. The primary entry points areTcpStream::connect()andTcpListener::accept(), so provideconnect_with_interest()andaccept_with_interest().