test: cover immediate client disconnects for ListenConnections#310
test: cover immediate client disconnects for ListenConnections#310xyzconstant wants to merge 2 commits 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. |
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
38391eb to
92d8f86
Compare
|
Thanks for the review @ViniciusCestarii!
Nice catch! Now it's clear why this wasn't failing on other platforms.
You're right on this, I was considering this too. I'm yet to figure out which exceptions we want to stop the accept loop for and which we want to retry, so a follow-up is the right call to me. If other reviewers would rather it be solved in this PR, I'm happy to tackle it here though. |
92d8f86 to
d8e5395
Compare
There was a problem hiding this comment.
Code review 38391eb
Nice catch and elegant test! However, I think the suggested fix is too dangerous because retrying whenever accept fails could lead to an infinite loop if the accept failed for reasons other than this macos/capnproto bug, like file descriptors being closed, or resources being exhausted.
I think it'd be good to add the test but not the fix here, and make the test allow the setsocketopt(IPPROTO_TCP, TCP_NODELAY): Invalid argument error (https://github.com/bitcoin-core/libmultiprocess/actions/runs/29545518652/job/87776862017#step:4:126) that results on macos when the capnproto version is less than whatever capnproto version fixes this to document the bug. Separately we should make sure this is fixed in the capnproto version we are using in Bitcoin Core and consider adding a patch to the Bitcoin Core depends build if needed temporarily.
| // Connect and close before the server has a chance to accept() the | ||
| // connection. | ||
| int fd = server.listener.MakeConnectedSocket(); | ||
| close(fd); |
There was a problem hiding this comment.
In commit "Keep listening after a failed accept in ListenConnections" (38391eb)
Can this be checked with KJ_SYSCALL?
| // Connect and close before the server has a chance to accept() the | ||
| // connection. | ||
| int fd = server.listener.MakeConnectedSocket(); | ||
| close(fd); |
There was a problem hiding this comment.
In commit "Keep listening after a failed accept in ListenConnections" (d8e5395)
Would be good to add a comment here that this close is a little racy, if the socket not closed before accepted the test could appear to crash when there is still a problem.
|
Thinking back here, I agree with @ryanofsky. In my review I understimated the spin forever on permanent failure case and the fix truly belongs to KJ and not here because if we were to try to reliably distinguish a transient error from a non transient error here we would basically be doing what KJ was supposed to do |
Add `DefaultLogHandler` and a `log_handler` parameter so tests can provide their own handler and observe log output more easily.
d8e5395 to
5fc3064
Compare
xyzconstant
changed the title
ListenConnectionsListenConnections
Connect to the listening socket and `close()` the connection right away, note that this is racy and the connection might be accepted normally if the `close()` syscall doesn't win the race. Additionally, a custom log handler is added to catch "Uncaught exception in daemonized task." logs, generated by a Cap'n Proto bug that shows up on macOS, so CI does not fail on them.
5fc3064 to
b5b357f
Compare
|
Thanks for the reviews, addressed feedback and force-pushed: d8e5395 -> b5b357f (compare) re: #310 (review)
Fair concern and I think you're right. The fix was designed to pass the test, assuming this behavior would be tolerated. So it's better we drop it, even though it means the test has a workaround for this macOS bug, which I think we could be okay with. |
4 participants
Following on testing the reversed direction @ryanofsky suggested in #298, this PR adds a test to cover immediate client disconnects on the server side.
Additionally, this test adds
DefaultLogHandlerand alog_handlerparameter toListenSetup, allowing individual tests to observe logs by passing a custom log handler. The new test takes advantage of this by catching and skippingUncaught exception in daemonized task.logs.NOTE: an issue surfaced on the macOS job, the
accept()call inListenConnectionsfails for the closed connection, this is a Cap'n Proto bug as reported by @ViniciusCestarii in #310 (review) and its fix is available in the v2 branch.