Fix two races that tear down a healthy socket - #26
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two races that tear a healthy socket down, both found by root-causing an intermittent
ESHUTDOWN/ECONNABORTEDon a freshly opened connection while running the pure-Swift D-Bus test suite. Before this change that suite failed roughly one run in five; after it, 11 consecutive full runs pass.Both bugs share a shape: state is read at a different moment than the one the decision belongs to.
1. Lost wakeup in
wait(for:fileDescriptor:)waitsuspends on a continuation, but registers it with the socket from a separateTask:discard(_:detach:)drains the queue viadequeueAll(_:), also from a separate task. If those two interleave the wrong way,queueruns afterdequeueAllhas already drained everything, appending the continuation to a state that nothing will ever drain again. The caller then waits forever.That is bad on its own — a permanently suspended task — but the real damage is that the stranded waiter keeps holding a file descriptor number. The descriptor itself is closed, so the kernel is free to hand the same number to the next socket, and the stale waiter then registers interest against whichever socket now owns it.
SocketStatenow records the teardown error, andqueueresumes a late arrival with it immediately rather than enqueuing it.Measured directly: with the D-Bus client instrumented to report when its read loop failed to stop after
close(), this fired 4–7 times per test run, and every failing run was one where it fired.2. A stale
POLLHUPacted on after the socket connectsAn unconnected socket reports
POLLHUP—Socket.initregisters the descriptor with the event queue beforeconnect, so there is a window where every socket polls as hung up.The existing guard checked
isEstablishedbefore honoring a hangup, but it checked it in the task that runs after polling. So:POLLHUP— the socket has not connected yet.connectcompletes and callsmarkEstablished().isEstablished == trueand tears the socket down.The check cannot distinguish a stale observation from a real hangup, because by the time it runs the flag says "established" in both cases. The next operation on that socket then fails with
ESHUTDOWNfromsocket(for:), orECONNABORTEDfrom the drained queue.hangup(_:socket:)now re-polls the descriptor and only proceeds if it still reports a hangup.POLLHUPis level triggered, so a genuine hangup is reported again and is acted on exactly as before; only the stale observation is filtered.Captured in the act, with the harmful event and the harmless ones side by side:
The
fd 7line was immediately followed by the test failure. With the re-check in place the same event still occurs and is now correctly ignored.Note
Neither fix ships with a regression test. Both are timing races between three tasks with no injection point to drive them deterministically, and a test that merely opens and closes sockets in a loop would reproduce them only probabilistically. They are verified by the D-Bus suite, which exercises the pattern heavily: 1-in-5 failures before, 11 consecutive clean runs after.
The pre-existing failures on this repo — the force unwrap at
NetworkInterface.swift:58, and the fixed-port and fixed-path tests colliding with leftovers from earlier runs — are untouched here and fail the same way onmain.