Use non-blocking connect for TcpStream. #687
Conversation
Instead of spawning a background thread which is unaware of any timeouts but continues to run until the TCP stack decides that the remote is not reachable we use mio's non-blocking connect. mio's `TcpStream::connect` returns immediately but the actual connection is usually just in progress and we have to be sure the socket is writeable before we can consider the connection as established.
Following a suggestion of @stjepang we offer methods to check for read/write readiness of a `Watcher` instead of the previous approach to accept a set of `Waker`s when registering an event source. The changes relative to master are smaller and both methods look more useful in other contexts. Also the code is more robust w.r.t. wakeups of the `Waker` from clones outside the `Reactor`. I am not sure if we need to add protection mechanisms against spurious wakeups from mio. Currently we treat the `Poll::Ready(())` of `Watcher::poll_write_ready` as proof that the non-blocking connect has finished, but if the event from mio was a spurious one, it might still be ongoing.
There was a problem hiding this comment.
No objections to this patch, especially since CI passes and it fixes an actual bug. I'd love to have an extra test to prevent any regressions of the reported bug; but even without it that shouldn't block merging this patch.
Delegating to @stjepang for a full review though.
ghost
merged commit 
|
@yoshuawuyts @stjepang Would it be possible to release a version including this in the not-too-far future? (it doesn't need to be in the next hours/days, but not too far away either if possible) In Polkadot we're sometimes reaching a limit to the number of threads using async-std 1.4 (possibly because we're within docker), which causes a panic in |
|
@tomaka Ah yeah, for sure. Going to make some time next week to issue a new release. |
Instead of spawning a background thread which is unaware of any timeouts but continues to run until the TCP stack decides that the remote is not reachable we use mio's non-blocking connect.
mio's
TcpStream::connectreturns immediately but the actual connection is usually just in progress and we have to be sure the socket is writeable before we can consider the connection as established.TheWatcherandReactorAPIs have changed a bit to (a) allow registration together with an initial set ofWakers in order to avoid missing any wakeups that may occur before we even registered aWakerwithWatcher::{poll_read_with, poll_write_with}, and (b) to allow registration with readiness interests other thanPollOpt::all().Update: Following a suggestion of @stjepang we offer methods to check for read/write readiness of a
Watcherinstead of the approach in the previous paragraph to accept a set ofWakers when registering an event source. The changes relative to master are smaller and both methods look more useful in other contexts. Also the code is more robust w.r.t. wakeups of theWakerfrom clones outside theReactor.I am not sure if we need to add protection mechanisms against spurious wakeups from mio. Currently we treat the
Poll::Ready(())fromWatcher::poll_write_readyas proof that the non-blocking connect has finished, but if the event from mio was a spurious one, it might still be ongoing.Context: #678.