WebSocket client: hold the frames that arrive with the 101 while paused

Closed
RO
1mo ago
+436-65

Problem

  • A client WebSocket dispatches the frames that arrive in the same read as the 101 response while isPaused is true, for a pause() before open or inside onopen: 5 of 5 on 1.4.2, canary and main (ws and wss). The docs say "A pause before the connection opens takes effect once it does."
  • The upgrade client gives the bytes it read past the 101 (the handshake overflow) to the connected client, which parses them in a microtask (InitialDataTask, src/http_jsc/websocket_client.rs).

Fix

  • The native client keeps a paused flag. While it is set, the overflow waits in held_data. So does every byte that still reaches handle_data: the rest of a TLS read, or the next chunk of a proxy tunnel. resume() queues the microtask that parses held_data before fresh socket data.
  • A peer that ends the connection (reset, FIN, TLS end, tunnel close) gets held_data parsed first, because main delivers these bytes today. With no socket left, that parse writes nothing back. A Close frame in it, or a handler's close(), names the close code.
  • A pause() inside a message handler does not change.
  • Verified: test/js/web/websocket/websocket-pause.test.ts (26 new tests, 17 fail on 1.4.2). Also test/js/web/websocket/ and test/js/first_party/ws.

Background

  • C++ WebCore::WebSocket owns the JS state. The Rust WebSocket<SSL> owns the socket and parses frames.
  • pause() calls us_socket_pause, which removes the readable interest from the poll. Bytes in user space are not affected.
  • A paused plain socket does not see FIN. A paused TLS client still sees a close_notify that arrives in a read in progress.
Notes

Source. No user reported this. A fuzz run that compares the pause() docs with the behaviour found it, with a raw-frame peer that writes the 101 and five frames in one write.

Repro (any release with pause()): a net server answers the upgrade with 101 ... \r\n\r\n plus five text frames in one socket.write(). The client calls ws.pause() right after new WebSocket(). All five message events fire while ws.isPaused === true. With this change none fires until resume(), then all five arrive in order.

Parked bytes. main already parks the handshake overflow (InitialDataHandler owns the bytes until its microtask runs). This PR does not add a second parking place. The bytes move to one held_data buffer on the client, the task becomes a trigger without a payload, and InitialDataHandler is gone. The parse loop itself does not change: a pause() inside a message handler still lets the rest of that read through. Whether that should change is a separate decision and not part of this PR.

Why handle_data holds bytes while paused. Through a CONNECT proxy the TLS session lives in WebSocketProxyTunnel. It decrypts a flight in 64 KB chunks, and the chunks after the first reach handle_data synchronously, after the pause is already in effect. Without the hold, the four tunnel variants of the new 101 test fail (checked by removing the branch). A direct TLS read can do the same.

Why held_data stays bounded. The flag is set only when pause_stream() reports that the socket stopped reading. After that, only bytes that were in user space already can arrive, or the kernel queue that uSockets drains when an error event ends the connection (1.6 MB in a probe). That last case ends in handle_close at once, which parses the bytes.

Peer endings. On main a reset while paused makes uSockets drain the kernel queue, the parser dispatches every message, and then close 1006 fires. Holding these bytes and then dropping them in clear_data() would lose them, so handle_close (without a pending local close) and the tunnel's close parse held_data first. close() and terminate() still drop it, like they drop unread kernel data. Without a rule for the Close frame, wss with a Close frame and close_notify in the 101 flight reports 1006 under a pause, and main reports the peer's code. The rule is in "Writes during that parse" below. The existing "no socket" branch of send_close_with_body is unchanged.

Writes during that parse. handle_close and the tunnel close set peer_ended before they parse. While it is set, send_pong skips the write, send_close_with_body reports the code without a write (the peer's Close frame, or a close() that a message handler calls, as on main where that handler runs with the socket still open), and a send() from a message handler is dropped. Before that, a Ping among the held frames made send_pong report an abrupt close (no socket), and the frames behind it were lost. handle_end parses too, but the socket can still write there, so it sets no flag. I could not build a test that reaches handle_end with held bytes: an unpaused client parses the overflow inside did_connect, and uSockets defers the EOF of a paused socket until resume().

Checked by removing code, then running the new tests with real proxies: no parse in handle_close fails 3, no parse in the tunnel close fails 2, no hold in handle_data fails 4, no peer_ended check in send_pong fails 4, none in send() fails 4, none in send_close_with_body fails 2, none in close() fails 2, only the peer's Close frame naming the code fails 4.

Known and not from this PR. Through a CONNECT proxy, an unpaused client reports 1006 "Failed to write" when a Close frame arrives together with the end of the TLS session, also on 1.4.3-canary. The tunnel answers close_notify before it delivers the last bytes, so the echo cannot be written. The new close-code test skips the unpaused tunnel cases for that reason. The paused tunnel cases pass, because the held bytes are parsed without a write.

Test environment. WebSocket applies NO_PROXY to an explicit proxy option. With NO_PROXY=localhost,127.0.0.1 in the environment the proxy variants of this file connect directly. I ran the file both ways.

Related open PRs. #42974 (resume a paused socket on close) calls self.resume() from send_close_with_body. resume(&self) stays and now also clears the flag, so the two changes compile together in either order. There is a small text conflict in pause(). #42976 (docs for a paused client) and #42978 (isPaused getter) do not overlap: this PR changes no docs, types or C++. #40458 changes the Drop of the same microtask type, so one of the two needs a mechanical rebase.

Review follow-up. The multi-line comments are one line each now. resume() from JS holds a ref on the client, because a resume that cannot re-arm the poll closes the socket. The Ping case above came from the review. The 101 tests reject on close and error.

Self-review. A first version also stopped the parse loop at a pause() inside a handler and changed the docs. The review found that it dropped held messages on a reset, broke #42974, repeated #42976, and changed a documented behaviour without a maintainer decision. This version keeps only the bug fix, adds the peer-ending rule, and leaves the rest out.

Suites on a debug (ASAN) build, all pass: websocket-pause (30), websocket-client, websocket-client-short-read, websocket-close-fragmented, websocket-pong-fragmented, websocket-permessage-deflate, websocket-permessage-deflate-edge-cases, websocket-close-code, websocket-close-async-dispatch, websocket-close-connecting, websocket-blob, websocket-unix, websocket-handshake-event, websocket-upgrade, websocket-syscall-fault, websocket-subprotocol-strict, websocket-custom-headers, error-event, websocket-server-send-from-drain, websocket-proxy, websocket-proxy-tunnel-client-leak, websocket-proxy-tunnel-upgrade-leak, websocket-proxy-close-reentrancy, websocket-buffered-amount, test-ws-bidir-proxy, and first_party/ws (ws, ws-proxy, ws-syscall-fault, ws-upgrade-events). A probe with 1500 random-size messages and random pause() / resume() calls from handlers, microtasks and timers kept the order in 8 configurations (ws and wss, with and without permessage-deflate).


[human-review] gate passed · iteration 2 · 3 files touched

fails on main (without fix)
ASAN without fix: 13 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/web/websocket/websocket-pause.test.ts
bun test v1.4.3 (c6b7fcb5b)

test/js/web/websocket/websocket-pause.test.ts:
(pass) WebSocket.pause() / resume() (ws) > stops reads while paused and drains after resume [2159.88ms]
(pass) WebSocket.pause() / resume() (ws) > pause() and resume() are idempotent [37.10ms]
(pass) WebSocket.pause() / resume() (wss) > stops reads while paused and drains after resume [2641.24ms]
(pass) WebSocket.pause() / resume() (wss) > pause() and resume() are idempotent [35.45ms]
(pass) WebSocket.pause() / resume() (wss via http:// proxy) > stops reads while paused and drains after resume [2536.84ms]
(pass) WebSocket.pause() / resume() (wss via http:// proxy) > pause() and resume() are idempotent [35.94ms]
(pass) WebSocket.pause() / resume() (wss via https:// proxy) > stops reads while paused and drains after resume [2827.51ms]
(pass) WebSocket.pause() / resume() (wss via https:// proxy) > pause() and resume() are idempotent [53.60ms]
(pass) WebSocket.pause() / resume() (ws via https:// proxy) > stops reads whil
... (truncated)

release without fix: 6 FAILED
bun test v1.4.3-canary.1 (7c17d346c)

test/js/web/websocket/websocket-pause.test.ts:
(pass) WebSocket.pause() / resume() (ws) > stops reads while paused and drains after resume [183.29ms]
(pass) WebSocket.pause() / resume() (ws) > pause() and resume() are idempotent [2.69ms]
(pass) WebSocket.pause() / resume() (wss) > stops reads while paused and drains after resume [303.40ms]
(pass) WebSocket.pause() / resume() (wss) > pause() and resume() are idempotent [2.31ms]
(pass) WebSocket.pause() / resume() (wss via http:// proxy) > stops reads while paused and drains after resume [302.26ms]
(pass) WebSocket.pause() / resume() (wss via http:// proxy) > pause() and resume() are idempotent [3.69ms]
(pass) WebSocket.pause() / resume() (wss via https:// proxy) > stops reads while paused and drains after resume [312.76ms]
(pass) WebSocket.pause() / resume() (wss via https:// proxy) > pause() and resume() are idempotent [3.01ms]
(pass) WebSocket.pause() / resume() (ws via https:// proxy) > stops reads while paused and drains after resume [161.43ms]
(pass) WebSocket.pause() / resume() (ws via https:// proxy) > pause() and resume() are idempotent [2.41ms]
(pass) WebSocket.pause() b
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/web/websocket/websocket-pause.test.ts
bun test v1.4.3 (c6b7fcb5b)

test/js/web/websocket/websocket-pause.test.ts:
(pass) WebSocket.pause() / resume() (ws) > stops reads while paused and drains after resume [1852.69ms]
(pass) WebSocket.pause() / resume() (ws) > pause() and resume() are idempotent [33.74ms]
(pass) WebSocket.pause() / resume() (wss) > stops reads while paused and drains after resume [2391.35ms]
(pass) WebSocket.pause() / resume() (wss) > pause() and resume() are idempotent [34.95ms]
(pass) WebSocket.pause() / resume() (wss via http:// proxy) > stops reads while paused and drains after resume [2637.68ms]
(pass) WebSocket.pause() / resume() (wss via http:// proxy) > pause() and resume() are idempotent [54.35ms]
(pass) WebSocket.pause() / resume() (wss via https:// proxy) > stops reads while paused and drains after resume [2671.18ms]
(pass) WebSocket.pause() / resume() (wss via https:// proxy) > pause() and resume() are idempotent [39.87ms]
(pass) WebSocket.pause() / resume() (ws via https:// proxy) > stops reads whil
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 715ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/7] gen generated_host_exports.rs
generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 242 extern-C blocks audited
[1/7] cargo bun_runtime → libbun_runtime.a
warning: binary `bun_shim_impl` should have a kebab-case name
   |
 1 | /workspace/bun/build/release/rust-target/.../bun_shim_impl
   |                                              ^^^^^^^^^^^^^
   |
   = note: `cargo::non_kebab_case_bins` is set to `warn` by default
help: to change the binary name to `bun-shim-impl`, convert `bin.name`
  --> src/install/windows-shim/Cargo.toml:41:8
   |
41 - name = "bun_shim_impl"
41 + name = "bun-shim-impl"
   |
warning: `bun_shim_impl` (manifest) generated 1 warning
warning: `feature(generic_const_exprs)` is not supported with
... (truncated)
diff hotspot
src/http_jsc/websocket_client.rs                   | 205 +++++++++-----
 .../websocket_client/WebSocketProxyTunnel.rs       |   2 +-
 test/js/web/websocket/websocket-pause.test.ts      | 294 +++++++++++++++++++++
 3 files changed, 436 insertions(+), 65 deletions(-)

gate history · 2 passed · 0 rejected · iteration 2

evidence per changed file
file                                                   reads  edits  tests
src/http_jsc/websocket_client.rs                          10      8     39
src/http_jsc/websocket_client/WebSocketProxyTunnel.rs      1      0     39
test/js/web/websocket/websocket-pause.test.ts              2      4     39