TcpListener: drain all pending accepts #224
Conversation
|
Good catch! Could you add a test? |
teskje
force-pushed
the
tcp-accept-drain-all
branch
from
139f6a8 to
652ed46
Compare
|
For sure, added one now! |
teskje
force-pushed
the
tcp-accept-drain-all
branch
from
652ed46 to
0841c40
Compare
src/net/tcp/listener.rs
Outdated
| let host = world.current_host_mut(); | ||
| let (syn, origin) = host.tcp.accept(self.local_addr)?; | ||
| let (syn, origin) = host.tcp.accept(self.local_addr).ok_or(Retry::Wait)?; |
There was a problem hiding this comment.
Could we return from the World::current closure here and avoid needing the enum? accept -> None signals the wait, syn.ack.send() -> err signals the retry, and the Ok path remains the same.
There was a problem hiding this comment.
Just to check my understanding: You are suggesting that we shorten the World::current closure to already end here and then do the syn.ack.send() and testing of the result outside of it, where we can continue the loop if necessary?
That would work, but in the Ok case we're have to enter World::current again to get access to the host back, unless I'm missing something. Still sounds a bit simpler, I'll give it a try!
There was a problem hiding this comment.
Pushed a change that gets rid of the enum and breaks out of the loop once we know we don't need to retry anymore. Lmk what you think!
This commit makes `TcpListener::accept` try to accept the next connection without waiting for a notify, if the previous connection couldn't be established because the peer already went away. The change is meant to fix a bug where a pending connection sometimes wouldn't be accepted. Consider this series of events: * Server runs `TcpListener::accept`. There are no connections pending, so it starts waiting on the `notify`. * Client 1 sends a SYN, which gets pushed to `ServerSocket::deque` and `ServerSocket::notify` is notified. * Client 2 sends a SYN, which gets pushed to `ServerSocket::deque` and `ServerSocket::notify` is notified. * Client 1 goes away. * Server wakes up because the `notify` was notified. * Server tries to accept the connection from client 1, which fails because client 1 went away. * Server starts waiting on the `notify` again. We end up in a situation where the connection from client 2 is still pending but the server doesn't accept it because it's waiting for a notification that was already sent.
teskje
force-pushed
the
tcp-accept-drain-all
branch
from
0841c40 to
5926783
Compare
This PR makes
TcpListener::accepttry to accept the next connection without waiting for a notify, if the previous connection couldn't be established because the peer already went away.The change is meant to fix a bug where a pending connection sometimes wouldn't be accepted. Consider this series of events:
TcpListener::accept. There are no connections pending, so it starts waiting on thenotify.ServerSocket::dequeandServerSocket::notifyis notified.ServerSocket::dequeandServerSocket::notifyis notified.notifywas notified.notifyagain.We end up in a situation where the connection from client 2 is still pending but the server doesn't accept it because it's waiting for a notification that was already sent.