Stop reading when the opening handshake fails.#1721
Conversation
|
I also traced the server-side rejection path for the same problem. When |
|
As you must have noticed, you're touching one of the trickiest areas of websockets' codebase! It's relatively recent and I'm willing to believe that not all error paths are handled correctly. I'll look carefully into your analysis and proposal. If you believe some of the content in the original issue is hallucinated (it's clearly AI-generated), please edit the description to make that clear 🙏 |
533cb50 to
f69ed34
Compare
|
I edited the description to spell out what I think doesn't hold up in the issue: the signal-based explanation. Signal delivery being main-thread-only is real and explains the traceback in #1592, but it's not why closing the socket fails to interrupt One nuance I found while probing: even though the second I also fixed the CI failure: |
Fix #1596.
I could reproduce the delay on macOS (Python 3.13) when the server rejects the handshake and keeps the TCP connection open, for both
ws://andwss://, at a 30-75% rate per attempt. The mechanism turned out a little different from the issue description.After parsing the non-101 response, the protocol sends EOF, so
send_data()half-closes the socket withshutdown(SHUT_WR)andrecv_events()blocks in arecv()bounded byclose_deadline, waiting for the server to close the connection. Whenclose_socket()then runs in the thread that calledconnect(), itsshutdown(SHUT_RDWR)is a secondshutdown()on a half-closed socket. On macOS that raisesENOTCONN, which the existingexcept OSError: passswallows, andclose()doesn't reliably interrupt arecv()that's already blocked.recv_events()rides outclose_timeout, or blocks forever withclose_timeout=None. On Linux the secondshutdown()succeeds, which is why this doesn't reproduce there.About the issue description: the explanation based on signals didn't hold up when I tested it. Signals are indeed delivered only to the main thread, which is why Ctrl-C in #1592 interrupted
join()rather thanrecv(), but waking a blockedrecv()withshutdown()andclose()doesn't involve signals. As long as the socket isn't already half-closed, it works reliably from any thread; I measured 20/20 prompt wakeups withrecv()blocked in a background thread and 10/10 with the roles reversed,recv()in the main thread and the closing calls in a background thread. The ordering described in the issue is real:recv_events()has to be blocked inrecv()beforeclose_socket()runs. What makes the wakeup fail is the prior half-close, not which threadrecv()runs on.I couldn't find any combination of
shutdown()/close()calls that interrupts the blockedrecv()reliably on macOS, so the fix removes the wait instead: when the opening handshake failed, the connection is unusable andclose_socket()runs as soon as the failure propagates, so there's no point waiting for the peer to close the connection.recv_events()stops reading and tears down the socket in its own thread, without depending on cross-thread wakeup. With this change I measured 0 delayed teardowns in 80 attempts, versus 30 in 40 before.The other scenario, where the server never responds and
open_timeoutfires, didn't hang in my tests: there's no prior half-close, so the firstshutdown()succeeds and interrupts the initialrecv()reliably. It's unchanged.The client test simulates the macOS
shutdown()/close()semantics at the socket level, so it fails without the fix on any OS, including Linux CI. The connection-level test exercises the new branch for both sides, which also keepsmaxi_covat 100%.