node:http: release body_read_ref when a response upgrades to a WebSocket
Closed
RO
1mo ago+111+111+012345678901234567890123456789-1-1-0123456789
Problem
- A
node:httpserver withwsupgrades 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) releasespoll_refbut keepsbody_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()releasesbody_read_reffor a body that is stillPending, next to thepoll_refrelease.- 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_statestaysPending, so a request whose body never arrives stays open withreq.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'stest-http-upgrade-*. Self-reviewed: 9 concerns raised, 8 addressed (Notes).
Background
body_read_refis ajsc::Ref: while held,vm.active_tasksis 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_stateisNone,PendingorDone. JS reads it throughhandle.hasBody, andIncomingMessage._readends the request onDone.wsreachesupgrade()throughserver.upgrade(res)inhandleUpgrade().
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
doneand exit in 0.15 s. Bun 1.4.0 and 1.4.3 canary printdoneand spin (timeout 20gives 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, thenonAbortat the close, which returns early. Nothing callsmaybe_stop_reading_bodyfor 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 callstick_without_idle(), a poll with a zero timeout.is_event_loop_alive()stays true because ofactive_tasks, so the outer loop never ends. A leakedjsc::Refwith 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.tsaborts 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 notPending. A held ref withDoneexists only while the lastondatacall is on the stack.on_data_or_abortedsetsDonebefore the call. After it, it releases the ref and runsmark_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_datasays that every site that releases the ref also moves the state out ofPendingor sets a flag that its guard rejects.upgrade()setsUPGRADED, which the guard rejects too. The comment now names it. - The first version also moved
body_read_statetoDone. A reader of the request that started after the upgrade then got'end'withreq.complete === truefor 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_refrelease tomark_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::unrefdoes 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-sidewsnever 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,activeTaskswas 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 thatwsrejects, 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 anUpgradeStream,req.complete === false, andreqstays 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:
reqgets the body,'end'and'close', then frames reachws. Bun: the body bytes reach the WebSocket parser,reqgets nothing, and a later frame is not delivered. - Body sent with the head. Node: the body is complete before
'upgrade'(req.complete === true, plainSocket). Bun:'upgrade'fires before the body is parsed. An upgrade inside the event drops the body (ws.test.tsdocuments 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 callsprocess.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: 1for three cases. The fourth one (later task, body sent) passes without the fix: the body is complete before the upgrade, soupgrade()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,wsandbun:internal-for-testing. Beside the child of thepoll_reftest, 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 deadws.on("error")handler, the assertion shape (one object withsignalCode), 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 newDropassertion on the three upgrade paths (inside the event, from a later task, from insideondata).
Unrelated
- In the debug build,
node-http-connect.test.ts> "tests should run on bun" times out at 5 s in this container. It spawnsbun testonnode-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