Conversation
http.Server and https.Server passed (null, hostname, port) to 'listening' listeners and to the listen() callback. Node's net.Server emits the event with no arguments, and so do bun's net, tls, http2 and dgram servers. Tests that read the port from the callback now use server.address().
|
Status: the fix is pushed and the PR is ready for review. How I reproduced it: const http = require("http");
const s = http.createServer(() => {});
s.on("listening", (...args) => console.log("listener", args));
s.listen(0, (...args) => {
console.log("callback", args);
s.close();
});
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe HTTP server now emits the ChangesListening callback arguments
Merge Risk: ⚪ Minimal · up to This intentionally aligns listening callbacks with Node’s zero-argument behavior. Bun-specific code that relied on the former host or port arguments must use server.address() instead. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the dropped reject(err) branches in test/js/third_party/st/st.fixture.ts and test/js/node/http/fixtures/http.compress.leak.server.ts — on the base branch the callback's first argument was always null (bind failures go through emitListenErrorNextTick as an 'error' event and never reach the listening callback), so no real rejection path was removed. The hard-coded localhost in the shared listen() helper in node-http.test.ts matches what the old callback passed, since this[serverSymbol].hostname was already the requested host rather than the bound address.
Extended reasoning...
The change is a one-emit compat fix in src/js/node/_http_server.ts (drop the Bun-only (null, hostname, port) from 'listening') plus mechanical test migrations to server.address().port; it touches no security-sensitive surface. Inline findings already flag the silent behavior break and the dropped express fetch timeout, so a human decision on the breaking change is needed; this note only records the sibling concerns that were examined and ruled out.
There was a problem hiding this comment.
The follow-up commit addresses the express hang-guard point from my earlier pass, and this run found no bugs in the code; since the PR deliberately removes a Bun-only callback signature with no deprecation path, a maintainer should still make the call on shipping that break (and the release-note line the description asks for).
What was reviewed:
- The single emit change in
src/js/node/_http_server.tsand itsnextTickcall site; greppedsrc/jsfor any remaining reader of the old(null, hostname, port)arguments —setupConnectionsTracking, the cluster listeners ininternal/cluster/*.ts, and thewsshim's bound re-emit all take no arguments, so nothing in-tree depends on them. - Grepped
test/for leftover(err, hostname, port)listen callbacks outsidetest/js/node/test/— none remain after the six updated files. - The new http/https/WebSocketServer assertions use exact
toEqual({ listener: [], callback: [] })and awaitclose, so they cannot pass vacuously against the old emit.
Extended reasoning...
The production change is two lines in src/js/node/_http_server.ts that make the node:http Server emit 'listening' with no arguments; the other seven files are tests and fixtures rewritten to read the port from server.address() plus three new assertions covering http, https, and the ws shim. It touches no security-sensitive surface. The code is small and I found no correctness issue, and the third commit resolved the express AbortSignal.timeout concern by branching on isDebug/isASAN. It is not approved outright because the change is an intentional silent removal of a long-standing Bun extension to the listen() callback, which is a user-facing API decision (with no warning added) that a maintainer should weigh rather than an automated reviewer.
|
Updated 9:07 AM PT - Sep 24th, 2026
✅ @robobun, your commit f75638446dbe8eaf3d5985fbbc3d7ae2ee4555b1 passed in 🧪 To try this PR locally: bunx bun-pr 43891That installs a local version of the PR into your bun-43891 --bun |
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; the express hang guard from the earlier round is now kept and widened per build type, so nothing is outstanding from my side. Because the one production edit is a deliberate break of a Bun-only listen() callback signature, a maintainer should still sign off on the compat decision and the release note.
What was reviewed:
- The single emit change in src/js/node/_http_server.ts against net.ts and the ws shim: both already forward or emit 'listening' with no arguments, and the cluster/connections-tracking listeners take no parameters.
- Checked the suite for remaining readers of the old
(err, hostname, port)arguments outside the diff — none left; thelocalhostsubstitution matches the value the callback previously received for the default host. - The new http/https and WebSocketServer cases assert whole
{listener, callback}objects withtoEqual, so they fail on the base with the three old arguments.
Extended reasoning...
The change drops the (null, hostname, port) arguments from the node:http 'listening' emit in src/js/node/_http_server.ts and rewrites seven test files to read server.address().port, plus adds http, https and ws regression tests. No security-sensitive surface is touched. The production diff is two lines and matches Node and Bun's own net/dgram servers, and the earlier express hang-guard concern was addressed in commit 06fa641. It is not approved outright because it is a silent user-observable breaking change of a long-standing Bun extension, which is a product decision for a maintainer rather than a correctness question.
Problem
http.Server,https.Serverand thewsWebSocketServerpass(null, hostname, port)to'listening'listeners and to thelisten()callback. Node passes nothing (emitListeningNT).async.waterfall([cb => server.listen(0, cb), next => ...])throwsTypeError: next is not a function.emitListeningNextTick(src/js/node/_http_server.ts:240). Handle errors in node:http better #12641 left// TODO: remove the argumentsabove it.Fix
emitListeningNextTickcallsself.emit("listening"), asnet.tsdoes. Thelisten()callback is aonce('listening')listener, so one emit covers both.server.address().port.test/js/node/http/node-http.test.ts(http, https) andtest/js/node/http/node-http-with-ws.test.ts(ws) fail on bun 1.4.2 and on a debug build of main with[null, "localhost", port]. Also rantest/js/node/http/, cluster, express, st, body-parser.Background
https.tsexports thehttp.Serverconstructor. Thewsshim re-emits its http server's'listening'with the same arguments (src/js/thirdparty/ws.js:1391). Both follow with no edit.listen()callback: the event keeps the wrong arguments. The emit is the only producer.Downsides
hostnameorportfrom the callback or the event now getsundefined. Useserver.address().port. Please add a release-note line.Notes
Repro:
Other shapes, same three runtimes (
async3.2.6,ws8.18.3 from npm on node, the built-in shim on bun):Node registers the callback with
this.once('listening', cb)(lib/net.js#L2120). bun does the same inServer.prototype.listensince #40041.History of the arguments:
node:httpserver, calledonListen(null, hostname, port).// TODO: remove the argumentsand// Note does not pass any arguments.above the emit.process.nextTick. No maintainer asked for that wording. The arguments are not indocs/orpackages/bun-types.dns.resolve(Fix dns.resolve callback parameters to match Node.js behavior #22814) andfs.exists(fix(node:fs): fixfs.existscallback parameters #6097)."(err, hostname, port) =>" listenand"(_err, host, port) =>" listenreturns only oven-sh/bun and copies of its test files.@types/noderejectslisten(0, (err, host, port) => ...).Test files that read the old arguments, all moved to
server.address().port:test/js/node/http/node-http.test.ts(9 sites, including thelisten()helper most of the file uses),test/js/node/http/fixtures/http.compress.leak.server.ts,test/js/third_party/express/express.test.ts,test/js/third_party/st/st.fixture.ts,test/js/third_party/body-parser/express-body-parser-test.test.ts,test/js/bun/test/parallel/test-http-10177-...ts(itsexpect(port).toBeGreaterThan(0)now readsserver.address().port). No code undersrc/jsreads them:setupConnectionsTrackingand the cluster worker listener take no parameters.express.test.tskeeps its hang guard for the express hang of #8926 and widens it on debug and ASAN builds:AbortSignal.timeout(isDebug || isASAN ? 5000 : 500). On a debug ASAN build the first request to express takes about 730 ms, so with the fixed 500 ms bound the test failed there with and without this change.Suites run on a debug build of this branch:
node-http.test.ts(165 pass, 1 skip), every other file intest/js/node/http/,test/js/node/cluster.test.ts(36 pass),test/js/bun/test/test-timers.test.ts,test/js/node/http/node-http-with-ws.test.ts,test/js/first_party/ws/ws.test.ts, express, st, body-parser, and 15 upstream files fromtest/js/node/test/parallel/(test-http-listening.js,test-http-server-close-all.js,test-https-simple.js,test-http-unix-socket.js,test-cluster-http-pipe.jsand others). I found no upstream node test that this change newly enables.Seen on the debug build and not related to this change:
test/js/node/http/node-http-syscall-fault.test.ts> "upRes.pipe(res) with res.destroy() racing a queued drain" needs 5.3 s against the 5 s default timeout. It passes with--timeout 120000.test/js/first_party/ws/ws-upgrade-events.test.tshas concurrent subprocess tests that time out at 5 s on a debug build. A different one fails on each run.test/js/first_party/ws/ws.test.tsfollowed bynode-http.test.tsin one debug-build process panics withassertion failed: !self.body_read_ref.get().has, with and without this change. node:http: release body_read_ref when a response upgrades to a WebSocket #43427 covers that bug. The ws case of this PR therefore lives innode-http-with-ws.test.ts.Open PR #42614 touches
emitListeningNextTickand passes the three arguments through a helper. Whichever lands second needs a small rebase.[human-review] gate passed · iteration 1 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file