Race condition in sync implemention delays closing connections

When this line:

websockets/src/websockets/sync/connection.py

Line 814 in 8c9f6fc

data = self.socket.recv(self.recv_bufsize)

runs before this line:

websockets/src/websockets/sync/client.py

Line 384 in 8c9f6fc

connection.close_socket()

closing the socket doesn't interrupt recv() ~~because recv() isn't running in the main thread — and interruption is based on signals which are only received by the main thread.~~ EDIT: #1721 says this is not the reason.

First discovered in #1592.

@scttbnsn

I believe I understand why the issue is happening.

First, for context, in WebSocket, the proper way for a client to close a connection is to half-close it then wait for the server to close it. The library is designed to do this, and that's why it calls recv(), expecting to receive EOF.

The problem discussed here occurs when a client attempts to connect and the server sends a non-101 response with Connection: keep-alive and does not close the connection.

In that case, websockets attempts to close the socket immediately but, as diagnosed by @scttbnsn, this does not interrupt recv() reliably on macOS.

Thanks a lot for clearing my misconceptions about the cause!

The fix proposed in #1721 feels awkward: I don't like the "make a note here then apply there" pattern, as it's difficult to reason about its correctness; more importantly, it's a symptom that we may be fixing at the wrong layer.

I've also been wondering if the same problem could happen elsewhere, and it can happen on the server during the opening handshake:

sync/server.py:751-758 — when the incoming bytes don't even parse as HTTP (ServerProtocol.parse(), server.py:598-608), send_eof() fires immediately, self.request stays None, so ServerConnection.handshake() skips its send_context()-guarded block entirely (sync/server.py:146) and raises directly. The handler thread's except TimeoutError/except Exception branches then call connection.close_socket() + connection.recv_events_thread.join() immediately, with no wait — structurally identical to client.py:387.

However, it doesn't seem possible to trigger the problem past the opening handshake — that would be during the closing handshake:

Why the opening-handshake case was vulnerable: client.py:387 calls close_socket() completely unconditionally, with zero relationship to any timeout — recv_events() might have just started a fresh, full-length close_timeout-bound recv() when the other thread barges in and tries to force it shut. There's no synchronization between "when the closer decides to give up" and "how much time is left on the blocked recv()."

Why the closing handshake doesn't have that problem: every close_socket() call reached during/after OPEN → CLOSING is gated behind send_context()'s wait_for_close logic (connection.py:1001-1023), and that logic and recv_events()'s own socket timeout (connection.py:824-825) both read .timeout() off the same self.close_deadline object — a single Deadline created once (connection.py:970-971 or :992-993) and shared. Deadline.timeout() (utils.py:42) just computes self.deadline - time.monotonic() against a fixed target instant, so no matter which thread asks, and no matter when, both are counting down to the identical wall-clock moment. That means recv_events()'s own blocking recv() will time out on its own, via the kernel's SO_RCVTIMEO, at essentially the same instant any other thread's join(timeout) would also expire — there's no window for one thread to need an external wakeup of an already-blocked recv(), which is specifically the thing that's unreliable on macOS. Any other thread (the closer, the keepalive thread) generally finds recv_events_thread already dead by the time its own wait elapses, or arrives at essentially the same moment recv_events() is timing out anyway.

A slightly more elegant fix might be:

                    if self.protocol.close_expected():

                        # ADDED THIS
                        # If we fail during the opening handshake, then we won't read more data anyway,
                        # and we never want to wait for the other side to close the connection as it could
                        # keep it open, e.g. an server sending an HTTP response with keep-alive enabled.
                        if self.protocol.state is CONNECTING:
                            self.close_deadline = Deadline(0)

                        if self.close_deadline is None:
                            self.close_deadline = Deadline(self.close_timeout)

This feels like a fix at the right level, because it cause recv() to return immediately in circumstances where we have good reasons to what that.

EDIT: actually it's not ideal:

side-effect I noticed while tracing this: it routes through the generic error handler, which sets recv_exc to a synthetic TimeoutError.

Maybe I'm slowly converging on the approach in the current PR...

I'll continue tomorrow :-)

Scott - thank you very much for your help in understanding the issue - probably I'll take it from there and write a test the way I want to write it :-)

No worries at all, glad it helped. Makes sense to take it from here and write the test the way you want it.

One thing that might be useful for the test: the delay isn't limited to the unparseable-request case — the normal process_request rejection (server returns a non-101, e.g. 403) has it too. I measured the server's reader thread riding out close_timeout (~302ms at close_timeout=300ms, same on main and this branch) when the client keeps the TCP connection open. Same "delays closing connections" symptom on the server side, independent of this PR.

Thanks for taking the time to work through it.

the normal process_request rejection (server returns a non-101, e.g. 403) has it too

Yes, this must be the most likely scenario triggering this problem.

The script is minimal_shutdown_repro.py. Here's the summary across all the runs so far, with RECV_TIMEOUT=1s:

┌─────────────────────────────────────────────────────────┬─────────────────────────────────────────────────────────┐
│                        Scenario                         │                         Result                          │
├─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┤
│ No half-close, shutdown()+close() from another thread   │ flaky (17–42% hang, varies run to run)                  │
├─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┤
│ Half-closed, then close() alone from another thread     │ 100% hang, every single trial, every run                │
├─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┤
│ Half-closed, then shutdown()+close() from another       │ flaky but clearly worse than no half-close (57–58%      │
│ thread                                                  │ hang)                                                   │
├─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┤
│ Half-closed, but the peer closes its end instead        │ 0% hang, every single trial, every run — always instant │
└─────────────────────────────────────────────────────────┴─────────────────────────────────────────────────────────┘

The two bolded rows are the proof: after shutdown(SHUT_WR), plain close() from another thread never wakes up the blocked recv() — not once across 50+ trials — while the peer actually closing its end always does, instantly, every time. That's the core claim demonstrated with a deterministic result, no flakiness needed. The shutdown()+close() combination (what real code typically does, hedging its bets) is in between and genuinely racy — which matches the flakiness we saw reproducing the actual websockets bug — but even at its best it's meaningfully worse once the socket has been half-closed first.

(all this on macOS)

Eventually I landed on the exact same fix as #1721. It's expressed a bit differently, but the behavior is the same.

This still needs a test. The proposal in #1721 "reproduces" in the connection layer by copy-pasting bits of the protocol layer, so we aren't testing the actual implementation; I'll see if I can do better.

The purpose of the test is mostly to exercise the new branch, and demonstrate that it's actually taken in the scenario discussed here.

Since the problem was never reproducible in a deterministic way, if we reintroduced the issue somehow, the test would become flaky instead of failing; I'm relying more on the comment than the test.

~~Hrm, after writing a test expected to reproduce the issue, the fix doesn't appear to work as intended :-(~~

Indeed, for a client, self.process_event(event) merely sets an event to notify the thread running handshake() that a response has been received. self.protocol.close_expected() still returns False until that other thread executes and find that the handshake failed. So we don't break out of the loop; we run recv() again; and we can have race conditions.

EDIT: I was just confused again :-(

We're really hitting a structural hard point with the design of our layers (Sans-I/O + I/O), which is really predicated on running recv() constantly an producing events, then interrupting it when we want to close the connection, in a layer that doesn't have protocol-specific logic.

Terminate quickly when opening handshake fails on macOS.

Port test added for #1596 to all implementations.

Revert #1596, which is unnecessary after #1777.

Port test added for #1596 to all implementations.

Revert #1596, which is unnecessary after #1777.

Port test added for #1596 to all implementations.

Revert #1596, which is unnecessary after #1777.

Port test added for #1596 to all implementations.

Revert #1596, which is unnecessary after #1777.

Navigation Menu

Repository navigation

Race condition in sync implemention delays closing connections #1596

11 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Race condition in sync implemention delays closing connections #1596

Description

Activity

darkhaniop commented on Apr 4, 2025

aaugustin commented on Apr 5, 2025

darkhaniop commented on Apr 5, 2025

darkhaniop commented on Apr 5, 2025

aaugustin commented on Jul 18, 2026

aaugustin commented on Jul 18, 2026

scttbnsn commented on Jul 18, 2026

aaugustin commented on Jul 19, 2026

aaugustin commented on Jul 19, 2026

aaugustin commented on Jul 19, 2026

aaugustin commented on Jul 19, 2026

aaugustin commented on Jul 19, 2026

11 remaining items

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions