Repository navigation
fix: stop a closed FakeServer from accepting one last connection - #25
Merged
Merged
Conversation
`test_a_refused_connection_provably_never_arrived` fails intermittently on CI with `Unknown: [Errno 104] Connection reset by peer; the request was written in full and the outcome is not known` - which is the opposite of what it asserts. The test wants a port nobody is listening on, and builds one by starting a FakeServer and closing it. But `close` set a flag and shut the socket without joining the accept thread, and that thread is parked inside `accept()`. Closing a socket from another thread does not reliably wake a thread already blocked there, so the listener could outlive `close` by however long it took to notice. A client connecting in that window is accepted by a server that was supposed to be gone; with an empty script the handler reads the request in full and returns, dropping the connection. The client has written everything and cannot know whether it landed, which is exactly `Unknown`. Fixed twice over, because the fixture bug is worth removing on its own: - The test binds a socket, takes its port and closes it without ever calling `listen`. Nothing can be accepted on a socket that never listened, so the refusal is not a race. - `FakeServer` now puts a timeout on the listening socket so the accept loop wakes to re-read its stop flag, and `close` joins the thread. After it returns, the server really has stopped - which is what every other test using it already assumed. Not a client bug: `Unknown` was the correct thing to report for what actually happened on the wire.
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.
test_a_refused_connection_provably_never_arrivedfails intermittently on CI withUnknown: [Errno 104] Connection reset by peer; the request was written in full and the outcome is not known- which is the opposite of what it asserts.The test wants a port nobody is listening on, and builds one by starting a FakeServer and closing it. But
closeset a flag and shut the socket without joining the accept thread, and that thread is parked insideaccept(). Closing a socket from another thread does not reliably wake a thread already blocked there, so the listener could outlivecloseby however long it took to notice. A client connecting in that window is accepted by a server that was supposed to be gone; with an empty script the handler reads the request in full and returns, dropping the connection. The client has written everything and cannot know whether it landed, which is exactlyUnknown.Fixed twice over, because the fixture bug is worth removing on its own:
listen. Nothing can be accepted on a socket that never listened, so the refusal is not a race.FakeServernow puts a timeout on the listening socket so the accept loop wakes to re-read its stop flag, andclosejoins the thread. After it returns, the server really has stopped - which is what every other test using it already assumed.Not a client bug:
Unknownwas the correct thing to report for what actually happened on the wire.