node:http: release body_read_ref when a response upgrades to a WebSocket

Closed
RO
1mo ago
+111-1

Problem

  • A node:http server with ws upgrades a request that declares a body inside the 'upgrade' event. After everything is closed, the process never exits and uses 100% CPU. A debug build panics: assertion failed: !self.body_read_ref.get().has (<NodeHTTPResponse as Drop>::drop).
  • upgrade() (src/runtime/server/NodeHTTPResponse.rs:527) releases poll_ref but keeps body_read_ref. Since #30875 the request is marked complete after the upgrade. The socket close path, which released the ref in 1.3, then returns early.

Fix

  • upgrade() releases body_read_ref for a body that is still Pending, next to the poll_ref release.
  • Correct because uWS destroys the HTTP response data, and its body callback, when it adopts the socket. A pending body never arrives, so the ref waits for nothing.
  • The request does not change. body_read_state stays Pending, so a request whose body never arrives stays open with req.complete === false, as on main and on Node v26.3.0. The first version of this PR ended the request at the upgrade. Review caught it.
  • Verified: test/js/node/http/node-http-with-ws.test.ts (new test, fails on bun 1.4.3 and on a debug build of main), test/js/first_party/ws/, node-http.test.ts, Node's test-http-upgrade-*. Self-reviewed: 9 concerns raised, 8 addressed (Notes).

Background

  • body_read_ref is a jsc::Ref: while held, vm.active_tasks is above zero and the event loop stays alive. A request that declares a body takes it. The last body chunk, req._dump() or a socket close releases it.
  • body_read_state is None, Pending or Done. JS reads it through handle.hasBody, and IncomingMessage._read ends the request on Done.
  • ws reaches upgrade() through server.upgrade(res) in handleUpgrade().
Notes

Repro (run from a directory that resolves ws, for example test/):

const http = require("node:http");
const net = require("node:net");
const { WebSocketServer } = require("ws");
const server = http.createServer();
const wss = new WebSocketServer({ server });
wss.on("connection", ws => ws.on("error", () => {}));
server.listen(0, "127.0.0.1", () => {
  const body = '{"hello":"world"}';
  const c = net.connect(server.address().port, "127.0.0.1", () =>
    c.write(
      "GET / HTTP/1.1\r\nHost: a\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n" +
        "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\nSec-WebSocket-Version: 13\r\n" +
        `Content-Length: ${body.length}\r\n\r\n${body}`,
    ),
  );
  c.on("data", () => c.destroy());
  c.on("close", () => {
    wss.close();
    server.close();
    console.log("done");
  });
});

The bug

  • Bun 1.3.5 and 1.3.14 print done and exit in 0.15 s. Bun 1.4.0 and 1.4.3 canary print done and spin (timeout 20 gives exit 124 with 19.9 s of user time). With an empty body every version exits.
  • getEventLoopStats() on the canary after everything is closed: activeTasks: 1, loopActive: false. With the fix: activeTasks: 0.
  • Trace of the unfixed build with BUN_DEBUG_NodeHTTPResponse=1: onRequestComplete, markRequestAsDone(), clearOnDataCallback, then onAbort at the close, which returns early. Nothing calls maybe_stop_reading_body for an upgraded response. The dispatch tail (src/runtime/server/mod.rs) only calls it when the response is not upgraded. The promise reactions that call it are not attached when the upgrade happened inside the handler.
  • In 1.3 the close path ran on_data_or_aborted(last = true), which released the ref. So the ref lived as long as the WebSocket.
  • The CPU use: auto_tick (src/runtime/jsc_hooks.rs) blocks in the poll only when the uSockets loop has an active handle. Otherwise it calls tick_without_idle(), a poll with a zero timeout. is_event_loop_alive() stays true because of active_tasks, so the outer loop never ends. A leaked jsc::Ref with no other handle behaves this way.
  • On a debug build of main, bun bd test test/js/first_party/ws/ws.test.ts test/js/first_party/ws/ws-upgrade-events.test.ts test/js/first_party/ws/ws-proxy.test.ts aborts the test runner with the assertion above after 87 passes. A later GC finalizes the responses that the tests with a request body leaked. With the fix: 100 pass, 0 fail.

The fix

  • upgrade() leaves a body alone that is not Pending. A held ref with Done exists only while the last ondata call is on the stack. on_data_or_aborted sets Done before the call. After it, it releases the ref and runs mark_request_as_done_if_necessary(). That accounting does not change.
  • An upgrade from a later task, before the body arrived, held the ref until the WebSocket closed. Now the upgrade releases it.
  • The comment in set_on_data says that every site that releases the ref also moves the state out of Pending or sets a flag that its guard rejects. upgrade() sets UPGRADED, which the guard rejects too. The comment now names it.
  • The first version also moved body_read_state to Done. A reader of the request that started after the upgrade then got 'end' with req.complete === true for a body that never arrived. Node keeps that request open, so the line is gone and the test now asserts the open request.
  • #38196 (open, conflicts with main) adds a body_read_ref release to mark_request_as_done() for another path. That line would also end this hang for an upgrade inside the 'upgrade' event. It does not cover the later-task upgrade. The two changes compose: Ref::unref does nothing when the ref is not held.
  • handleUpgrade() from inside the request's 'data' or 'end' handler: on main and with this change, the server-side ws never emits 'close' after the client disconnects. #43182 works on that path. With its uWS changes applied on top of the first version of this change, the socket closed, activeTasks was 0 after the upgrade and after the close, and forced GCs on a debug build passed.
  • Other teardown paths with a pending body do not leak on the canary: res.end() before the body completes plus a client disconnect, res.destroy(), req.destroy(), socket.destroy(), and an Upgrade with a body that ws rejects, that the listener destroys, or that the listener answers with a raw 101.

Node v26.3.0 compared (npm ws 8.18.3, raw client, Content-Length: 17, handleUpgrade() inside the 'upgrade' event and from setImmediate, reader of req attached right after handleUpgrade())

  • Body never sent. Node: 'upgrade' gets an UpgradeStream, req.complete === false, and req stays open after the 101. Bun, on main and with this PR: the same for the request. The difference: Node reads the next 17 bytes as the body, Bun hands them to the WebSocket as frames.
  • Body sent after the 101. Node: req gets the body, 'end' and 'close', then frames reach ws. Bun: the body bytes reach the WebSocket parser, req gets nothing, and a later frame is not delivered.
  • Body sent with the head. Node: the body is complete before 'upgrade' (req.complete === true, plain Socket). Bun: 'upgrade' fires before the body is parsed. An upgrade inside the event drops the body (ws.test.ts documents that). An upgrade from a later task delivers it.
  • These differences are on main too, and this PR does not touch them. To remove them, the WebSocket has to take the socket only after the request body is complete. That is a separate change, tracked in #43716 (related: #43182, #43149).

The test

  • One child process runs four upgrades of a request that declares a body: from a later task and inside the 'upgrade' event, each with the body sent and not sent. When the client has the 101, the child records the active tasks the upgrade left behind. Where the body is not sent, it also records that the request is still open ('end' not emitted, req.complete === false), which is what Node shows. The process has to exit by itself. It calls process.exit(1) only when it saw a leak, so that an unfixed build fails in 70 ms instead of a timeout.
  • Unfixed builds (bun 1.4.3, and a debug build of main) report leakedTasks: 1 for three cases. The fourth one (later task, body sent) passes without the fix: the body is complete before the upgrade, so upgrade() has nothing to release.
  • The first version of the child also passed with the environment of the CI ASAN lane (BUN_JSC_validateExceptionChecks=1, BUN_DESTRUCT_VM_ON_EXIT=1, ASAN_OPTIONS=...detect_leaks=1...): exit 0 and an empty stderr on the debug build.
  • The test is not concurrent. A debug build needs about 3 s to load node:http, ws and bun:internal-for-testing. Beside the child of the poll_ref test, both get slower on a loaded machine.

Self-review (three read-only passes: ref accounting, JS-visible effects, the test)

  • Addressed: the 'end' that the first version gave a late reader (removed after the Node comparison), the CI ASAN environment was not exercised, a request that is never answered made the child hang, the missing flag combinations, a dead ws.on("error") handler, the assertion shape (one object with signalCode), the comment style, and no test covered the request state.
  • Rejected: an assertion on the number of refs a response holds before the upgrade. It would pin an internal number.
  • The ref accounting pass found no double release, no missed mark_request_as_done() and no new Drop assertion on the three upgrade paths (inside the event, from a later task, from inside ondata).

Unrelated

  • In the debug build, node-http-connect.test.ts > "tests should run on bun" times out at 5 s in this container. It spawns bun test on node-http-connect.node.mts, which passes in 4.9 s when run directly.

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

fails on main (without fix)
ASAN without fix: 1 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/http/node-http-with-ws.test.ts
bun test v1.4.3 (367d939d9)

test/js/node/http/node-http-with-ws.test.ts:
(pass) request handlers run to completion before the callbacks they queued > the 'connection' handler of a WebSocketServer, run from the upgrade request [515.04ms]
(pass) request handlers run to completion before the callbacks they queued > a 'request' handler closing an open WebSocketServer socket [477.70ms]
(pass) should not crash when closing sockets after upgrade [1089.42ms]
(pass) WebSocket upgrade should unref poll_ref from response [2282.75ms]
253 |   expect({
254 |     results: stdout.startsWith("{") ? JSON.parse(stdout) : stdout,
255 |     stderr,
256 |     exitCode,
257 |     signalCode: proc.signalCode,
258 |   }).toEqual({
           ^
error: expect(received).toEqual(expected)

  {
-   "exitCode": 0,
+   "exitCode": 1,
    "results": {
      "in a later task, body not sent": {
-       "leakedTasks": 0,
+       "leakedTasks": 1,
        "requestOpen": true,
        "status": "HTTP/1.1 101 Switching Protocols",
... (truncated)

release without fix: 1 FAILED
bun test v1.4.3-canary.1 (367d939d9)

test/js/node/http/node-http-with-ws.test.ts:
(pass) request handlers run to completion before the callbacks they queued > the 'connection' handler of a WebSocketServer, run from the upgrade request [14.30ms]
(pass) request handlers run to completion before the callbacks they queued > a 'request' handler closing an open WebSocketServer socket [12.45ms]
(pass) should not crash when closing sockets after upgrade [39.24ms]
(pass) WebSocket upgrade should unref poll_ref from response [43.31ms]
253 |   expect({
254 |     results: stdout.startsWith("{") ? JSON.parse(stdout) : stdout,
255 |     stderr,
256 |     exitCode,
257 |     signalCode: proc.signalCode,
258 |   }).toEqual({
           ^
error: expect(received).toEqual(expected)

  {
-   "exitCode": 0,
+   "exitCode": 1,
    "results": {
      "in a later task, body not sent": {
-       "leakedTasks": 0,
+       "leakedTasks": 1,
        "requestOpen": true,
        "status": "HTTP/1.1 101 Switching Protocols",
      },
      "in a later task, body sent": {
        "leakedTasks": 0,
        "status": "HTTP/1.1 101 Switching Protocols",
      },
      "in the 'upgrade' event, body 
... (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/node/http/node-http-with-ws.test.ts
bun test v1.4.3 (367d939d9)

test/js/node/http/node-http-with-ws.test.ts:
(pass) request handlers run to completion before the callbacks they queued > the 'connection' handler of a WebSocketServer, run from the upgrade request [551.60ms]
(pass) request handlers run to completion before the callbacks they queued > a 'request' handler closing an open WebSocketServer socket [513.76ms]
(pass) should not crash when closing sockets after upgrade [1167.37ms]
(pass) WebSocket upgrade should unref poll_ref from response [2278.56ms]
(pass) WebSocket upgrade should unref body_read_ref from response [2919.37ms]

 5 pass
 0 fail
 8 expect() calls
Ran 5 tests across 1 file. [8.14s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     41cf4bda76
  features     lto, baseline

23 deps, 136 codegen, 1176 objects in 732ms

ninja: Entering directory `/workspace/bun/build/release'
[1/4] fetch lolhtml
[lolhtml] up to date
[2/4] fetch rust-argon2
[rust-argon2] up to date
[2/4] cargo plan → /workspace/bun/build/release/rust-target/plan.json
244 units: 172 lib, 16 proc-macro (host), 19 custom-build (host), 15 run custom-build, 17 lib (host), 4 run custom-build (host), 1 rlib
[3/4] reconfigure
[1/1499] mkdir stamps
[2/1499] mkdir codegen
[3/1499] install /workspace/bun
bun install v1.4.3-canary.1 (367d939d9)

Checked 26 installs across 65 packages (no changes) [10.00ms]
[4/1499] install /workspace/bun/packages/bun-error
bun install v1.4.3-canary.1 (367d939d9)

Checked 1 install across 2 packages (no changes) [1.00ms]
[5/1499] install /workspace/bun/src/node-fallbacks
bun install v1.4.3-canary.1 (367d939d9)

Checked 111 installs across 104 packages (no changes) [7.00ms]
[6/1499] esbuild bun-error

  ../../build/release/codegen
... (truncated)
diff hotspot
src/runtime/server/NodeHTTPResponse.rs      |   6 +-
 test/js/node/http/node-http-with-ws.test.ts | 106 ++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 1 deletion(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file
file                                         reads  edits  tests
src/runtime/server/NodeHTTPResponse.rs          12      7     39
test/js/node/http/node-http-with-ws.test.ts      6      5     39