Skip to content

Stop reading when the opening handshake fails.#1721

Open
scttbnsn wants to merge 1 commit into
python-websockets:mainfrom
scttbnsn:fix/1596-sync-close-socket-race
Open

Stop reading when the opening handshake fails.#1721
scttbnsn wants to merge 1 commit into
python-websockets:mainfrom
scttbnsn:fix/1596-sync-close-socket-race

Conversation

@scttbnsn

@scttbnsn scttbnsn commented Jul 3, 2026

Copy link
Copy Markdown

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:// and wss://, 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 with shutdown(SHUT_WR) and recv_events() blocks in a recv() bounded by close_deadline, waiting for the server to close the connection. When close_socket() then runs in the thread that called connect(), its shutdown(SHUT_RDWR) is a second shutdown() on a half-closed socket. On macOS that raises ENOTCONN, which the existing except OSError: pass swallows, and close() doesn't reliably interrupt a recv() that's already blocked. recv_events() rides out close_timeout, or blocks forever with close_timeout=None. On Linux the second shutdown() 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 than recv(), but waking a blocked recv() with shutdown() and close() 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 with recv() 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 in recv() before close_socket() runs. What makes the wakeup fail is the prior half-close, not which thread recv() runs on.

I couldn't find any combination of shutdown()/close() calls that interrupts the blocked recv() reliably on macOS, so the fix removes the wait instead: when the opening handshake failed, the connection is unusable and close_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_timeout fires, didn't hang in my tests: there's no prior half-close, so the first shutdown() succeeds and interrupts the initial recv() 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 keeps maxi_cov at 100%.

@scttbnsn

scttbnsn commented Jul 3, 2026

Copy link
Copy Markdown
Author

I also traced the server-side rejection path for the same problem. When process_request rejects the handshake, send_context() half-closes the socket from the handler thread, and its forced close_socket() after close_timeout is again a second shutdown(), so the swallowed ENOTCONN happens there too. I couldn't turn it into a hang though: recv_events() is blocked in an untimed recv() on that path and close() woke it in 30/30 attempts on macOS, so this PR leaves it alone.

@aaugustin

aaugustin commented Jul 3, 2026

Copy link
Copy Markdown
Member

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 🙏

@scttbnsn scttbnsn force-pushed the fix/1596-sync-close-socket-race branch from 533cb50 to f69ed34 Compare July 3, 2026 21:12
@scttbnsn

scttbnsn commented Jul 3, 2026

Copy link
Copy Markdown
Author

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 recv(). shutdown() + close() wakes a blocked recv() from any thread, unless the socket was already half-closed by the EOF the protocol sends after parsing the rejection. That prior half-close is the ingredient the issue was missing.

One nuance I found while probing: even though the second shutdown() fails with ENOTCONN, attempting it still makes the subsequent close() much more likely to wake the blocked recv() on macOS (93% vs 0% in isolated probes when skipping it entirely). So the swallowed call in close_socket() is doing useful work and I left it alone.

I also fixed the CI failure: maxi_cov measures sync/connection.py against test_connection.py alone, and the new branch was only exercised from test_client.py. I added a connection-level test that simulates a failed opening handshake the way ClientProtocol.parse leaves the protocol, runs for both sides, and fails without the fix by riding out close_timeout. Amended into the same commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in sync implemention delays closing connections

2 participants