Unset the keep-alive timer when upgrading to WebSocket

Merged
KA
2mo ago
+21-10

Summary

handle_websocket_upgrade() returns before the _unset_keepalive_if_required() that guards ordinary requests, so a WebSocket upgrade pipelined behind a plain request leaves the HTTP keep-alive timer armed on a transport that now belongs to the WebSocket protocol.

The upgrade is picked up by the handle_events() call at the end of on_response_complete() — that is, immediately after the timer was armed — so it never goes through data_received(), which would have unset it.

Reproducing

Send a plain request and an upgrade in one segment, then hold the WebSocket open past timeout_keep_alive:

GET / HTTP/1.1\r\nHost: example.org\r\n\r\nGET /ws HTTP/1.1\r\nHost: example.org\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Key: <key>\r\nSec-WebSocket-Version: 13\r\n\r\n
Exception in callback H11Protocol.timeout_keep_alive_handler()
h11._util.LocalProtocolError: can't handle event type ConnectionClosed when role=SERVER and state=SEND_RESPONSE

That is the message reported in #1637 and discussed in #1984; both stalled for want of a reproducer, so this is at least one concrete path to it.

Why not simply catch the error

Because the raise is load-bearing today:

if not self.transport.is_closing():
    event = h11.ConnectionClosed()
    self.conn.send(event)     # raises
    self.transport.close()    # never reached

Wrapping the send in try/except h11.LocalProtocolError and letting transport.close() run turns a noisy log line into dropped connections — I measured the live WebSocket being closed timeout_keep_alive seconds after the upgrade. Releasing the timer at the hand-over keeps both properties: no exception, and the WebSocket stays up.

Scope

h11_impl and zttp_impl both reach handle_websocket_upgrade() from on_response_complete().

httptools_impl is deliberately left unchanged: it upgrades only from within data_received(), which unsets the timer first, and it arms the timer only when its pipeline is empty, so the state is unreachable there. The test skips it with that reason.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.