Skip to content

node:http: run a response's 'finish' connection step on the socket the dispatcher detached it from - #44457

Open
robobun wants to merge 2 commits into
mainfrom
robobun/95bc534a/http-finish-detached-socket
Open

robobun wants to merge 2 commits into
mainfrom
robobun/95bc534a/http-finish-detached-socket

Conversation

@robobun

@robobun robobun commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A 'request' listener that runs the stream destroyer on req (Readable.toWeb(req).cancel()) and calls res.end() in the same tick ends the process: TypeError: undefined is not an object (evaluating 'res') at onResponseFinishHandleSocket.
  • emitResponseFinish (src/js/node/_http_server.ts:2617) reads this.req?.socket ?? this.socket. The destroyer nulled req.socket and the dispatcher already detached the response, so the getter builds a FakeSocket with no .server.
  • Without a throw, a Connection: close response leaves the connection open.

Fix

  • The dispatcher stores socket where it stored true (kDispatcherDetached, now kDetachedFrom). emitResponseFinish runs the connection step on it.
  • A response that ends later uses its own slot. req.socket and req.client stand in only when user code emptied or replaced it.
  • writableHighWaterMark reads that slot, so a detached response builds no FakeSocket.
  • Verified: node-http-server-timeouts.test.ts (6 new), node-http.test.ts (31 new: 29 scenarios, 26 equal to node v26.3.0). Also 34485.test.ts and the node-http-* files.

Background

Downsides

  • util.inspect(res) of a same-tick response prints 11027 characters, 6493 before: the slot holds the socket.
  • emitResponseFinish has 79 bytecode instructions, 60 before. The builtin grows 357 bytes.
Notes

Reach. Bun 1.4.0 and later. 1.3.14 and Node.js are not affected: 1.3.14 bound the socket into the listener, and #32488 replaced that with the shared listener. #34488 added the flag that this PR turns into a value. Node main does the same since nodejs/node#65802: one shared 'finish' listener that reads the connection from a private slot of the response.

Repro. The server below answers 401 three times on Node.js. On Bun the first request ends the process with exit code 1.

import http from "node:http";
import { Readable } from "node:stream";
const server = http.createServer((req, res) => {
  const body = Readable.toWeb(req);
  if (req.headers.authorization !== "secret") {
    body.cancel();
    res.statusCode = 401;
    res.end("no");
    return;
  }
  res.end("ok");
});

Every way in. The same 'finish' failed for reader.cancel(), stream.destroy(req), an aborted pipeline(), compose().destroy(), Duplex.from(req).destroy(), req.socket = null, req.socket = undefined, req.connection = null and a foreign req.socket, from 'request', 'checkContinue', 'checkExpectation' and 'dropRequest', over http, https and a unix socket. node-http-finish-connection-scenarios.mjs has one scenario for each, and runs under Node.js too. Unfixed Bun gives 7 of the 26 Node rows and 22 uncaught exceptions.

Why the listener has two arms. With one shared tail, the arm that never ran in a process is still compiled (a get_by_val that never ran has no profile and is not pruned). The socket variable is then untyped at the merge and the DFG code grows from 304 nodes and 9 Branch nodes to 366 and 15. With two arms each keeps its own types. The request fallback is guarded by && req, so that block starts with a property read that never ran and is compiled as an exit. The fallback also takes a replaced res.socket whose server is null, the shape of the socket of Node.js's net module.

Measurements. main 4b02e10 against the same diff. Debug builds for bytecode and heap, release builds for DFG, FTL and size. Handlers: res.end() in the listener (same-tick), setImmediate(() => res.end()) (later-tick), res.write(); res.end() (write+end).

  • own keys per response: 48 at 'request', 50 / 52 at close (main 48, 50 / 52; Reflect.ownKeys)
  • bytecode instructions: emitResponseFinish 79 (main 60), onNodeHTTPRequest 749 (main 749), writableHighWaterMark getter 14 (main 12)
  • executed bytecodes per request: same-tick 5020 (main 5026), later-tick 5116 (main 5118), write+end 5488 (main 5848). JS frames per request: 239 / 245 / 260 (main 239 / 245 / 273). BUN_JSC_traceBaselineJITExecution=1, requests 2 to 5 of a keep-alive connection.
  • emitResponseFinish in the optimising tiers, 45000 keep-alive requests, BUN_JSC_useConcurrentJIT=0: DFG same-tick 273 nodes and 7 Branch nodes (main 304 and 9), later-tick 315 and 9 (main 314 and 8). FTL 195 / 255 Air instructions with onResponseFinishHandleSocket inlined (main 128 / 174, plus 107 / 109 in the callee that main compiles apart). The release build has no disassembler, so the unit is DFG nodes and Air instructions.
  • dispatcher store: PutByOffset, PutStructure and 1 FencedStoreBarrier, 43 bytes of DFG code and 5 Air instructions (main 49 bytes, 5 Air instructions, 1 barrier)
  • instructions per keep-alive request, end to end: not measured. perf_event_open is denied in the build container and valgrind is not installed.
  • heap cells kept per retained response: res.end 20.0 (main 19.9, the same types), write+end 19.99 (main 31.88). FakeSocket placeholders per 200 responses: 0 / 0 / 0 (main 0 / 200 / 200) for end, write+end, and a writableHighWaterMark read in 'finish'.
  • server sockets alive while 1000 finished responses are retained: 1000 (main 1000); after dropping them: 0 (main 0). The request already keeps the socket reachable.
  • builtin _http_server.js: +357 bytes; release binary text: +357 bytes
  • util.inspect(res) at 'close': 11027 chars (main 6493)
  • idle connections reaped, 100 keep-alive clients at keepAliveTimeout 300: 100 of 100 (node 100, main 0). socket.timeout in 'finish': 300 (node 300).

Other suites. node-http-displaced-response, node-http-with-ws, node-http-server-close-drain, and node's test-http-*.js and test-stream-destroy.js (392 files: 389 pass; test-http-agent-keepalive.js and test-http-client-timeout-option.js fail on the debug build with and without this diff and pass on both release builds).

Left for later.

  • A body that still arrives after the response does not refresh the keep-alive timer when the request was destroyed. The connection then closes keepAliveTimeout after 'finish'. This is what main already does when the response ends in a later tick, and node:http: refresh the socket inactivity timer on reads that reach no JS callback #43744 changes it. Node.js reads the body to its end first.
  • res.socket read by user code after the listener returns is a FakeSocket in Bun and null in Node.js.
  • A response that ended in the listener's tick is detached before 'finish'. Node.js keeps its socket until 'finish'.
  • Expect: 100-continue answered from 'checkContinue' without writeContinue(): Node.js closes the connection, Bun keeps it.
  • A socket.server that user code set to undefined still makes 'finish' throw, as on main.

Open PRs near these lines. #43461, #37974, #35664, #43744, #43441, #37724.

Not run. macOS, Windows.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 4 billable files and costs up to $1.00.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 21 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: 77e9f45b-5df3-450a-a885-4d2eba3a1039
📥 Commits

Reviewing files that changed from the base of the PR and between aa83076 and 8a3ab1c.

📒 Files selected for processing (4)
  • src/js/node/_http_server.ts
  • test/js/node/http/node-http-finish-connection-scenarios.mjs
  • test/js/node/http/node-http-server-timeouts.test.ts
  • test/js/node/http/node-http.test.ts
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Oct 2, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/js/node/_http_server.ts
Comment thread test/js/node/http/node-http-server-timeouts.test.ts Outdated
Comment thread test/js/node/http/node-http.test.ts Outdated
@robobun

robobun commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on Bun 1.4.3-canary.1+367d939d9 (Linux x64) with the script in the Notes of the description. The first request ends the process with exit code 1:

TypeError: undefined is not an object (evaluating 'res')
      at onResponseFinishHandleSocket (node:_http_server)
      at emitResponseFinish (node:_http_server)

Node.js v26.3.0 answers 401 three times and stays alive.

The new tests fail on that build (USE_SYSTEM_BUN=1 bun test test/js/node/http/node-http-server-timeouts.test.ts, and node-http.test.ts -t "links to its socket were cut"). On this branch bun bd test passes both files.

…rom, and 'finish' runs its connection step on that record

A 'request' listener that runs the stream destroyer on req (for example
Readable.toWeb(req).cancel()) and ends the response in the same tick made
the 'finish' listener read a placeholder socket: the destroyer had set
req.socket to null and the dispatcher had already detached the response.
The listener then threw a TypeError from node:_http_server, or skipped the
close of a response that said Connection: close.

The dispatcher now stores the socket in the slot where it stored true, and
emitResponseFinish uses that socket for a response the dispatcher detached.
A response that still owns its socket uses its own slot, and falls back to
the request's links only when user code emptied or replaced that slot.
ServerResponse#writableHighWaterMark reads the same slot, so it no longer
builds a placeholder socket for a detached response.
@robobun
robobun force-pushed the robobun/95bc534a/http-finish-detached-socket branch from 5ad9082 to 04a98b1 Compare October 3, 2026 08:19
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
@robobun

robobun commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:04 AM PT - Oct 3rd, 2026

✅ @robobun, your commit 8a3ab1cc79b516dfbc2931dd7ccacad4df11506c passed in Build #123287! 🎉


🧪   To try this PR locally:

bunx bun-pr 44457

That installs a local version of the PR into your bun-44457 executable, so you can run:

bun-44457 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

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.

2 participants