Skip to content

WebSocket client: queue bytes that arrive while the frame parser is on the stack - #48554

Draft
robobun wants to merge 1 commit into
mainfrom
robobun/1b8e418a/websocket-client-reentrant-parse
Draft

robobun wants to merge 1 commit into
mainfrom
robobun/1b8e418a/websocket-client-reentrant-parse

Conversation

@robobun

@robobun robobun commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #48524

Problem

  • A message listener that runs a nested event loop tick (Bun.build() with an async plugin setup(), a macro) makes the WebSocket client read the socket again from inside the frame parser. With a frame split across two TCP reads the client then dispatches a phantom empty message, and with permessage-deflate the next frame fails with Invalid compressed data (close 1002, 1006 on main).
  • The cause is handle_data_loop (src/http_jsc/websocket_client.rs). It keeps the parse cursor on the stack and writes it back to self only when it returns. The nested handle_data started a second loop from the stale state (NeedBody, 0 bytes left) and called consume with an empty message.

Fix

  • The parser gets one entry, ingest(). A call that arrives while a parse is on the stack queues its bytes in initial_data. The outer call parses them, in order, after its own loop returns. feed() (the JS stream transport) already had this guard. handle_data, handle_tunnel_data and parse_initial_data now share it.
  • While bytes are queued the socket is paused, so a listener that waits for long does not buffer the peer's stream without bound. The queue is resumed once parsed.
  • A close that arrives while the parser is on the stack (handle_close, handle_tunnel_close, the close timeout) is reported after the bytes already read are parsed. terminate() from a listener stops reading at once, so nothing read before it is delivered after it.
  • Verified: test/js/web/websocket/websocket-client-short-read.test.ts, eight new cases (text and permessage-deflate), all fail on Bun 1.4.2 and pass with this change. Also all of test/js/web/websocket/ and test/js/first_party/ws/.

Background

  • uSockets reads into one receive buffer per loop and lends a slice of it to the socket's on_data handler. The handler may run JS. JS may tick the loop again (wait_for_promise), which dispatches the same socket again before the outer handler returns.
  • RecvCursor is that handler's parse state: the receive state, the bytes left in the frame, and the rest of the read. Each step of the loop runs listeners through dispatch_data, so the cursor is live during every listener.
  • Considered persisting the cursor to self before each listener: the nested loop still runs, its result is clobbered when the outer loop writes its cursor back, and it adds stores per message. Considered a per-depth receive buffer in uSockets so the rest of the outer read survives a nested tick: that makes the loop tolerate re-entry, which event loop: never re-enter from inside a dispatch callback (de-block expect().resolves and HTMLRewriter.transform, then assert) #33261 rules out, so it is not in this PR. The queue here costs nothing per message and is the shape feed() and the h2 frame parser already use.

Downsides

  • A listener that nests the loop now sees the frames the nested tick read after it returns, not inside the nested tick. That is the order feed() already had and the order Node's ws has.
  • Per TCP read on the common path: one parsing load and branch, two stores, two Cell::take. No new allocation. Binary size and allocation counts: MEASURE_PLACEHOLDER.
  • The rest of the outer read still lives in the shared receive buffer. A nested tick that reads any other socket overwrites it, as before this PR. event loop: never re-enter from inside a dispatch callback (de-block expect().resolves and HTMLRewriter.transform, then assert) #33261 tracks the removal of nested ticks from I/O callbacks, which is what makes that impossible.
Notes
  • Trigger inventory. Bun 1.4.4 and earlier: expect(p).rejects.toThrow() in bun test (the issue's repro). bun test: Vitest compatibility #44833 removed that on main. On main: Bun.build() with an async plugin setup() (src/runtime/api/JSBundler.rs), which Stop running the event loop inside JavaScript calls #46545 removes, and macros that return a promise, which stay.
  • The phantom message also fires without permessage-deflate. With deflate the nested consume of an empty message also runs decompress on empty input, which feeds only the 00 00 ff ff trailer to the shared inflate stream. The next real frame then fails with Z_DATA_ERROR.
  • cancel() (terminate()) calls stop_reading() when a parse is on the stack. Then handle_close finishes at once and the outer loop returns on Closed. Outside a parse nothing changes: ws mode still parses the bytes that arrived with the 101.
  • stream_closed (JS stream transport) is not deferred: ws emits close as soon as it returns, so queued bytes would be dispatched on a closed socket. It finishes at once and drops the queue, as before.
  • Bytes queued before a listener calls pause() stay in initial_data. resume() delivers them through the existing deliverInitialData task. A connection that already ended parses them regardless, as handle_close did before.
  • Suites run with the debug build: test/js/web/websocket/ (54 files, 3929 pass, the two ws.postman-echo.com tests fail for lack of network), test/js/first_party/ws/, test/js/bun/net/socket.test.ts, test/js/node/net/node-net.test.ts, test/js/bun/http/serve.test.ts.
  • Siblings with the same shape that this PR does not touch: the Bun.serve WebSocket parser in uWS (Finalize WebSocket frame state before the frame's callback runs #46093), node IPC advanced mode (src/runtime/ipc.rs), the HTTP server body parser. The h2 frame parser already has its own queue (rewrite_tail).
  • Self-reviewed: 21 concerns raised, 9 addressed (close deferred on the parse, not on the queue; stream_closed not deferred; terminate() stops reading; queue backpressure; tests for FIN with nothing queued and for terminate(); a marker that pins the nested tick; the uSockets half dropped per event loop: never re-enter from inside a dispatch callback (de-block expect().resolves and HTMLRewriter.transform, then assert) #33261). Rejected: a wss:// test variant and a depth-3 free-list test (both for the dropped uSockets half), an unbounded-queue concern (the pause addresses it).

…n the stack

A message listener can run a nested event loop tick (Bun.build() with an
async plugin setup, a macro). The nested tick reads the socket again and a
second handle_data_loop ran over the outer loop's half-updated state. With
a frame split across two reads that dispatched a phantom empty message
and, with permessage-deflate, fed an empty message to the shared inflate
stream so the next frame failed with Invalid compressed data.

The parser now has one entry, ingest(), which queues bytes that arrive
while it is on the stack and parses them after the outer loop returns.
This is the guard feed() already had for the JS stream transport. While
bytes are queued the socket is paused, so a listener that waits for long
does not buffer the peer's stream without bound. A close that arrives
while the parser is on the stack is reported after the bytes already read
are parsed, and terminate() from a listener stops reading at once.
@robobun

robobun commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:17 PM PT - Nov 10th, 2026

⏳ @robobun, your commit 2ea3cec is still building in Build #140051, but has 1 failures so far (All Failures):

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebSocket permessage-deflate fails after a split frame resumes a promise continuation

2 participants