Keep listening after a failed accept() in ListenConnections#310
Keep listening after a failed accept() in ListenConnections#310xyzconstant wants to merge 1 commit into
ListenConnections#310Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste |
xyzconstant
changed the title
ListenConnections
|
the macos job failed with: I came across the same issue on my machine (MacBook Pro M2) too. I'll push a fix in a moment. |
A failed `accept()` would stop the server from accepting any further connections, because the accept loop was only continued after a successful accept. Handle this by also keeping the accept loop running on failures.
f44094f to
38391eb
Compare
xyzconstant
changed the title
ListenConnectionsListenConnections
ViniciusCestarii
left a comment
ViniciusCestarii
left a comment
There was a problem hiding this comment.
ACK 38391eb Nice finding! This is nicer than the old code that would just stop listening because of a transient error that KJ didn't catch but also with this change now a non-transient error will spin forever retrying an unrecoverable error which I believe could be fixed in a follow up.
Also I have dug in and I found that this is a KJ bug because it should treat transient errors like this and it was already fixed for capnp v2 which isn't released yet.
Given that, I still think this PR is correct and the defensive retry is the right call regardless. The spin forever on permanent failure case remains a valid follow-up on top of this.
| // Keep listening if a single accept() fails, so the server does not | ||
| // stop accepting future connections. | ||
| [&loop, &init, listener](const kj::Exception& e) { | ||
| MP_LOG(loop, Log::Info) << "IPC server: accept failed."; |
There was a problem hiding this comment.
In "Keep listening after a failed accept in ListenConnections" 38391eb
nit: I believe MP_LOG(loop, Log::Warning) would fit better here and also the kj exception could be logged too
3 participants
Following on testing the reversed direction @ryanofsky suggested in #298, I was working on adding tests to cover immediate client disconnects on the server side.
While doing so, an issue surfaced on macOS: the
accept()call inListenConnectionsfails for the closed connection, causing the server socket to stop accepting further connections. The root cause is that the accept loop is only continued after a successfulaccept(), on failure the exception is unhandled, so the loop is never resumed.This PR handles the failure and keeps the accept loop running. Also, it adds a test covering the case.