net: use &self with TcpListener::accept #2919
Conversation
Uses the infrastructure added by #2828 to enable switching `TcpListener::accept` to use `&self`. This also switches `poll_accept` to use `&self`. While doing introduces a hazard, `poll_*` style functions are considered low-level. Most users will use the `async fn` variants which are more misuse-resistant. TcpListener::incoming() is temporarily removed as it has the same problem as `TcpSocket::by_ref()` and will be implemented later.
| @@ -186,33 +186,78 @@ impl ScheduledIo { | |||
| } | |||
| } | |||
|
|
|||
| /// Notifies all pending waiters that have registered interest in `ready`. | |||
There was a problem hiding this comment.
The changes here solve a potential deadlock during shutdown. Waking a task must be done from outside of the lock or it could result in a deadlock due to an attempt to re-enter the lock.
| @@ -132,8 +132,19 @@ impl Registration { | |||
| cfg_io_readiness! { | |||
| impl Registration { | |||
| pub(super) async fn readiness(&self, interest: mio::Interest) -> io::Result<ReadyEvent> { | |||
| // TODO: does this need to return a `Result`? | |||
There was a problem hiding this comment.
It turns out that it does need to return a result :)
This implementation is not great but I believe the shutdown logic needs to be revisited post 0.3.
There was a problem hiding this comment.
I tracked my thoughts in #2924
|
@seanmonstar I wrote some quick thoughts on I also encountered a few bugs that were introduced with the intrusive waker change. I fixed them in here and highlighted the fixes with comments. |
| /// | ||
| /// When ready, the most recent task that called `poll_accept` is notified. | ||
| /// The caller is responsble to ensure that `poll_accept` is called from a | ||
| /// single task. Failing to do this could result in tasks hanging. |
There was a problem hiding this comment.
Should eventually include language that poll_accept should only be used for 1 task.
|
👋 I came across this PR because I have troubles upgrading from tokio 0.2 to tokio 0.3.
I see that the function was removed - but I don't think it's a good design to "temporary remove" public API methods that dozens of other projects are relying on. I'd love to see tokio as a mature library in a way that doesn't come with surprises in "temporary removed" stuff. Have you considered going through a deprecation cycle and keeping it in 0.3 with a warning that in future versions it might get removed? |
|
@kirs With Tokio 0.3 the As for a deprecation cycle, I don't think it's worth it for such easily replaced functionality such as this. Especially not when we are releasing Tokio v1.0 shortly, with which we guarantee no breaking changes for several years. Tokio v0.3 was always intended so we can try out the final version of the APIs before we put them in stone for v1.0.0, and temporarily removing APIs so we can add them back without a breaking change in v1.x if we are not sure about them is an important step to be able to do this. |
Uses the infrastructure added by #2828 to enable switching
TcpListener::acceptto use&self.This also switches
poll_acceptto use&self. While doing introducesa hazard,
poll_*style functions are considered low-level. Most userswill use the
async fnvariants which are more misuse-resistant.TcpListener::incoming() is temporarily removed as it has the same
problem as
TcpSocket::by_ref()and will be implemented later.