node:net: emit 'listening' on the next tick so close() from the handler never inherits an accepted connection - #39114
node:net: emit 'listening' on the next tick so close() from the handler never inherits an accepted connection#39114robobun wants to merge 5 commits into
Conversation
net.Server emitted 'listening' from setTimeout(1), so the event loop polled the new listening socket, and accepted whatever had connected, before user code ran the 'listening' handler. Code that probes for a free port by listening and closing from that handler (vite's tryListen, get-port, detect-port) therefore ended up owning an accepted connection it never reads. Since accepted sockets got Node's half-open semantics, such a connection no longer goes away when the peer hangs up, and server.close() waits on it forever; `bun --bun vite dev` hung at startup whenever something connected during the probe. Bun.listen() has already called listen(2) when kRealListen returns, so emit on process.nextTick like Node's setupListenHandle does. close() from the handler then closes the fd before the loop ever polls it and the kernel resets the pending peer, which is what Node does too. The cluster round-robin worker path gets the same treatment.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review (head fba573f). CI is red only on tests unrelated to this change. Reproduced on a main release build with vite 8.2.1 ( The first CI run failed upstream Latest build (98610): 176/179 jobs green, including every darwin, Windows, musl and aarch64 lane. The 3 red jobs fail only on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a two-site setTimeout(..., 1) → process.nextTick swap that aligns 'listening' ordering with Node and closes the vite port-probe hang. The one inline finding is a test-convention nit with no failure mode.
What was reviewed:
- Confirmed
Bun.listen()sets_handlesynchronously inkRealListen, so the old "must run after IO poll" comment no longer applies andemitListeningNextTick'sif (!self._handle)guard still covers close-before-emit. - Checked both call sites (
kRealListen,kClusterFauxListen) were updated;_http_server.tsintentionally left as-is per the PR description. - Second test relies on
server.address()being available immediately afterlisten(0, "127.0.0.1")andBun.connectissuingconnect(2)synchronously — both hold in Bun, so the backlog race is deterministic.
Extended reasoning...
Overview
Two runtime lines changed in src/js/node/net.ts: Server.prototype[kRealListen] and Server.prototype[kClusterFauxListen] now schedule emitListeningNextTick via process.nextTick instead of setTimeout(..., 1). The stale comment (which predated Bun.listen() binding synchronously) is replaced with one explaining why nextTick is correct and linking Node v26.3.0's setupListenHandle. Two regression tests are added to test/js/node/net/node-net-server.test.ts: one asserts 'listening' fires before a nextTick queued after listen(), and one asserts a peer that lands in the backlog before the 'listening' handler closes the server is never accepted (accepted === 0).
Security risks
None. This is pure event-ordering in the Node compat layer — no auth, crypto, parsing, or untrusted-input handling is touched.
Level of scrutiny
Low-to-medium. The runtime diff is two identical one-line substitutions that move Bun closer to Node's documented and source-linked behavior. The mechanism is fully explained (nextTick runs before the first IO poll, so close() from the handler shuts the listening fd before any backlog connection is accepted; us_listen_socket_close closes synchronously so the kernel resets pending peers). emitListeningNextTick already guards on !self._handle, so a close-before-emit still suppresses the event, matching Node's emitListeningNT. I verified _handle is assigned synchronously in every kRealListen branch (unix path, fd, port), so address() is valid immediately and the new comment is accurate.
Other factors
The PR description documents that the full test/js/node/{net,tls,cluster*} suites and 428 upstream test-net-*/test-tls-*/test-cluster-* files behave identically before and after, and that both new tests fail on main and match Node 26.3.0. _http_server.ts still uses a 1ms timer — explicitly scoped out because http serves whatever it accepts and has no equivalent hang. The single bug-hunter finding is a nit (missing server.on('error', reject) in the second test); its own extended reasoning shows a listen failure would fail fast via the server.address() destructure rather than hang, and listen(0, "127.0.0.1") cannot realistically fail on CI. No CODEOWNERS covers these files. Approving.
With 'listening' on the next tick, a server listened and closed from a 'beforeExit' handler is gone again before the tick drain returns, so nothing is alive and 'beforeExit' is not re-emitted. In Node the handle's uv_close() completes on the next loop turn, which keeps the loop alive until then (test-process-beforeexit relies on it). Queue a no-op immediate from close() for the same effect, as closeSocketHandle already does for sockets, and cover it with a test. Also wire the listen probe test's 'error' event to its promise.
|
Updated 2:06 PM PT - Aug 15th, 2026
❌ @robobun, your commit fba573f has 3 failures in
🧪 To try this PR locally: bunx bun-pr 39114That installs a local version of the PR into your bun-39114 --bun |
There was a problem hiding this comment.
I reviewed this PR and found no bugs; the earlier nit about wiring 'error' → reject in the second new test was addressed in 1b29469. Because the change reverses a prior in-code warning that process.nextTick() is not sufficient here and pairs it with a compensating setImmediate(noop) in close(), a maintainer with context on the original setTimeout choice should confirm the "Bun.listen() has already called listen(2)" claim before this lands.
What was reviewed:
emitListeningNextTickstill guards on!self._handle, so a close before the tick skips the emit (matches Node'semitListeningNT).setImmediate(noop)inclose():noopexists at net.ts:164; mirrorscloseSocketHandle's pattern; libuv keeps closing handles alive one turn regardless of ref state, so this doesn't over-hold for unref'd servers.emitErrorNextTickon the listen-failure path is stillsetTimeout(..., 1)— unchanged, so error ordering vs. the new nextTick-based'listening'is unaffected (they're mutually exclusive).- New tests: ordering test can fail (both entries pushed before assertion); backlog test wires
'error'to reject and'listening'handler is attached afterlisten()but before any await, so the nextTick hasn't run yet; beforeExit test drains stdout+stderr concurrently.
Extended reasoning...
Overview
Three production-code edits in src/js/node/net.ts: (1) Server.prototype[kRealListen] and (2) Server.prototype[kClusterFauxListen] switch from setTimeout(emitListeningNextTick, 1, this) to process.nextTick(emitListeningNextTick, this), and (3) Server.prototype.close adds setImmediate(noop) after this._handle.stop(false) to hold the loop for one turn the way Node's uv_close does. Three new tests in test/js/node/net/node-net-server.test.ts cover nextTick ordering, the vite-style listen-then-close-from-handler pattern with a peer already in the backlog, and the beforeExit re-emit that the setImmediate(noop) preserves.
Security risks
None. This is event-loop timing in the Node net compat layer; no auth, crypto, parsing of untrusted input, or filesystem paths are touched.
Level of scrutiny
Medium-high. The diff is tiny but changes when 'listening' fires relative to the first I/O poll for every net.Server, and it explicitly deletes a comment that said "process.nextTick() is not sufficient because it will run before the IO queue." The PR argues that comment predates Bun.listen() synchronously calling listen(2), cites Node's setupListenHandle (lib/net.js L2034-L2037), and reports the full test/js/node/{net,tls,cluster*} suites plus 428 upstream test-net-*/test-tls-*/test-cluster-* files behave identically. That is thorough, but the class of change — reversing a documented timing decision and adding a compensating loop-hold in close() — is exactly where a maintainer's historical context is valuable.
Other factors
- The
setImmediate(noop)is a second-order fix: without it, moving'listening'to nextTick would regress upstreamtest-process-beforeexit(the old 1ms timer incidentally held the loop). The PR adds a dedicated test for this and the comment cites the upstream test by name, so it isn't hidden. It fires on everyserver.close(); libuv'suv__loop_alivecounts closing handles regardless of ref state, so this matches Node for unref'd servers too. node:http's server is intentionally left onsetTimeout(..., 1); the PR notes it doesn't share the hang. That leaves a deliberate inconsistency betweennetandhttplisten timing that a maintainer may want to weigh in on.- My previous review's only finding (missing
'error'→rejectwiring in the backlog test) was addressed in 1b29469 and the thread is resolved. This run's bug-hunting pass found nothing further.
|
On the "Bun.listen() has already called listen(2)" point, for whoever picks this up: History of the timer, since a maintainer may remember it: #2337 originally emitted On |
|
Independently hit the same hang while checking
The repro below needs no external traffic, so it can be run against any build (and against node). It connects the peer itself right after repro.mjsimport net from "node:net";
// Pick a port first so the peer can connect before 'listening' has been emitted.
const port = await new Promise(resolve => {
const probe = net.createServer().listen(0, "127.0.0.1", () => {
const { port } = probe.address();
probe.close(() => resolve(port));
});
});
const server = net.createServer();
let closeFired = false;
server.on("connection", () => console.log("accepted a connection (node never gets here)"));
server.once("listening", () => server.close(() => { closeFired = true; console.log("close() callback fired"); }));
server.listen(port, "127.0.0.1");
const client = net.connect(port, "127.0.0.1", () => {
client.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
setTimeout(() => client.end(), 200);
});
client.on("error", () => {});
setTimeout(() => {
console.log(closeFired ? "ok" : "HANG: close() never completed after the client went away");
process.exit(closeFired ? 0 : 1);
}, 1500);
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/net.ts:3576-3581— comment-cop flagged both new multi-line comments (here and atkRealListen, line ~3975). The bot's generic wording reads as "fix the code", but thesetImmediate(noop)is load-bearing — the fix is to condense each comment to one line, e.g. here:// Hold the loop one turn like Node's async uv_close (test-process-beforeexit); same as closeSocketHandle.ThecloseSocketHandleprecedent at ~4294 and the upstream test name carry the rest.Extended reasoning...
What was flagged
The repo's automated
comment-coplint posted two unresolved inline comments on this PR:src/js/node/net.ts:3580— the 5-line comment (3576-3580) justifyingsetImmediate(noop)inServer.prototype.closesrc/js/node/net.ts:3975— the 6-line comment (3970-3975) justifyingprocess.nextTick(emitListeningNextTick, this)inkRealListen
Both trip CLAUDE.md rule 13 ("If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code") and REVIEW.md's code-style rule ("Only comment what the code cannot say. One line. … Prefer links to GitHub issues.").
Why this isn't just a duplicate of the bot's flag
The bot's message is the raw rule text and can be misread as "delete the workaround". For the
close()site that would be wrong: removingsetImmediate(noop)regresses upstreamtest-process-beforeexit(and the newnode-net-server.test.tscase), because Bun'sstop()closes the listening fd synchronously whereas Node'suv_close()keeps the loop alive one more turn. The comment is load-bearing — without itsetImmediate(noop)looks like dead code and will eventually be deleted. So the actionable resolution the bot doesn't spell out is: keep the code, shrink the comment.Why one line is enough
closeSocketHandleatnet.ts:4289-4296already establishes the exact same pattern for sockets (handle.close(...)followed bysetImmediateto hold the loop for the turn Node's asyncuv_closeprovides). Referencing that precedent plus the upstream test name is sufficient for the next reader to reconstruct the full reasoning — the PR description and the linked Node source line already record the long-form explanation.Suggested one-liner for 3576-3580:
// Hold the loop one turn like Node's async uv_close (test-process-beforeexit); same as closeSocketHandle. setImmediate(noop);
And for 3970-3975 in
kRealListen, the Node source link on the last line already does most of the work; the prose above it can collapse to something like:// Bun.listen() has already called listen(2); emit on nextTick like Node so close() from the handler runs before the first accept poll. // https://github.com/nodejs/node/blob/v26.3.0/lib/net.js#L2034-L2037 process.nextTick(emitListeningNextTick, this);
Step-by-step
net.ts:3576-3580is a 5-line prose comment above a one-line statement → matches the comment-cop heuristic → bot posts on line 3580.- REVIEW.md requires "One line" for what-the-code-cannot-say comments; 5 lines > 1 → violation.
closeSocketHandle(net.ts:4294) already usessetImmediateafterhandle.close()for the identical uv_close-turn reason, so "same as closeSocketHandle" is a lossless pointer.- The upstream test name
test-process-beforeexitis grep-able and pins the exact behaviour being preserved. - Therefore the 5 lines compress to 1 without information loss, clearing the lint while keeping
setImmediate(noop)explained.
Impact & severity
Style/convention only — no runtime effect. The repo's own linter flagged it, so it needs addressing to clear CI annotations, but it does not change behaviour. nit.
There was a problem hiding this comment.
I re-reviewed after the reject wiring and comment-length fixes — no bugs found. Given this changes when 'listening' fires relative to the first I/O poll for every net.Server, and reverts 25097cd whose original motivation couldn't be reconstructed, a maintainer sign-off would still be worthwhile.
Checked: noop and emitListeningNextTick's !self._handle guard are already in the file; closeSocketHandle already uses setImmediate the same way; the two remaining comment-cop flags look stale (both comments are one-liners at head fba573f).
Extended reasoning...
Overview
Three effective lines in src/js/node/net.ts: kRealListen and kClusterFauxListen switch from setTimeout(emitListeningNextTick, 1, this) to process.nextTick(emitListeningNextTick, this), and Server.prototype.close gains setImmediate(noop) after _handle.stop(false) to keep the loop alive one turn like Node's uv_close(). Three new tests in node-net-server.test.ts cover ordering vs. nextTick, the vite-style listen-then-close-from-'listening' backlog case, and the beforeExit re-emission.
Security risks
None. No untrusted input handling, no auth/crypto, no new user-facing surface. The change is purely event-loop scheduling.
Level of scrutiny
High. node:net's Server.listen/close is on the path of every TCP server, and this alters when user 'listening' handlers run relative to the loop's first accept poll. It also reverts a deliberate 2023 commit (25097cd) that switched nextTick → 1ms timer; the PR author git-blamed it, found no test, and could not reconstruct what it was papering over. The analysis is thorough (native listen traced synchronous, Node's setupListenHandle uses nextTick, upstream net/tls/cluster/process suites run), but the "couldn't reconstruct why the timer was added" point is exactly the kind of thing a maintainer with 2023 context should confirm.
Other factors
- My earlier nit (wire
'error'→rejectin the backlog test) was addressed in 1b29469; verified in the current diff. - Verified
noopexists (net.ts:164) andemitListeningNextTickstill guards on!self._handle(net.ts:4009-4012), so aclose()betweenlisten()and the tick suppresses the emit as in Node. - The
setImmediate(noop)mirrors the existing pattern incloseSocketHandle(net.ts:4285), so it's consistent with the file, though it now fires on everyserver.close()— cheap but worth a maintainer nod. - Two comment-cop inline comments (19:15:22Z) are still open in the timeline, but both target comments are one-liners at head fba573f, so they appear stale rather than unaddressed.
- The author explicitly left
node:http's 1ms-timer'listening'emit alone as out of scope; that's a reasonable boundary for a regression fix but a maintainer may want it aligned.
2 participants
Problem
bun --bun vite dev(vite 8.2.1) intermittently never becomes ready on main: no output, main thread idle inepoll_wait, no listening socket. 1.3.14 always comes up. About 1 start in 10 when something polls the port during startup; more often on fewer CPUs.CLOSE_WAIT, and no listener. vite never got pastisPortAvailable()(tryListen()in vite'shttp.ts):net.createServer().listen(port, host), thenserver.close(cb)from the'listening'handler.cbnever fired.Server.prototype[kRealListen](src/js/node/net.ts:3971before this change) emitted'listening'fromsetTimeout(..., 1). The event loop therefore polled the new listening socket, and accepted whatever had connected to it (here: the health-check poller), before the'listening'handler ran.close()then found_connections > 0and waited for that connection, which nothing reads. Node emits'listening'fromprocess.nextTick(setupListenHandle, lib/net.js v26.3.0 L2034-L2037), so in Node the listening fd is closed before the loop ever accepts, and the kernel resets the pending peer. Verified: under the same connection hammering, Node reportsaccepted=0every time.allowHalfOpen: falsenatively, so the stray connection was torn down as soon as the poller gave up and the close callback fired (vite came up 1-3s late). net,tls: port Node.js net/tls compatibility tests and fix the gaps they surface — half-open/reset/write semantics, server TLSSocket wrap, session/keylog, SNICallback/ALPNCallback, pfx, OpenSSL error shapes, addCACert, local binding (+305 tests) #31155 gave accepted sockets Node's half-open semantics (a peer FIN with unread data leaves the socket open, as in Node), so the stray connection now lives forever andclose()never completes.Fix
kRealListenandkClusterFauxListenemit'listening'withprocess.nextTick(emitListeningNextTick, this)instead of a 1ms timer.Bun.listen()has already calledlisten(2)when it returns (Listener::listen->us_socket_group_listen->bsd_create_listen_socket, all synchronous;kRealListenreads the bound port back right after), so the event is not early;emitListeningNextTickstill skips the emit if the server was closed in between, like Node'semitListeningNT. The timer came from 25097cd (2023), which replaced feat(net.createServer) and adds socket.connect IPC support #2337's originalnextTickwithout a test; the native listen was already synchronous then and nothing in the current suites needs the delay (details in a comment below).close()from the handler now runs before the first poll of the listening fd;us_listen_socket_closecloses the fd synchronously, so connections still in the backlog are reset by the kernel and are never accepted, matching Node.Server.prototype.close()queues a no-opsetImmediatewhen it closes the native listener. In Node the handle'suv_close()completes on the next loop turn and the loop counts as alive until then; upstreamtest-process-beforeexitdepends on that (a server listened and closed from a'beforeExit'handler must make'beforeExit'fire again). The 1ms timer used to provide that turn by accident; with'listening'on a tick the server is gone before the tick drain returns, soclose()holds the loop for the turn itself, the same waycloseSocketHandlealready does for sockets.'close'itself is still emitted onnextTick, so its ordering is unchanged.test/js/node/net/node-net-server.test.ts:'listening'is ordered before anextTickqueued afterlisten(); a peer thatconnect(2)ed into the backlog before the'listening'handler closes the server is not accepted; closing a server listened from'beforeExit're-emits'beforeExit'. The first two fail on main (acceptedis 1 there), the third fails with thenextTickchange alone and passes with theclose()hold; all three match Node 26.3.0.curlloops hammering port 5173 during startup (forces a connection into the probe window): main release build hangs 3/3 with an empty log, 1.3.14 comes up 3/3 after 1-3s, this branch comes up 3/3.test/js/node/net,test/js/node/tls,test/js/node/cluster*, and the upstreamtest-net-*/test-tls-*/test-cluster-*/test-process-*/test-http2-*files plus every upstream file mentioningbeforeExit(787 files,test-process-beforeexit.jsincluded): same results as main (the remaining failures here arelocalhostresolution and load timeouts in this container that fail identically on main or pass when run alone).node:http's server still emits'listening'from a 1ms timer (_http_server.ts); it does not have this hang (it serves whatever it accepts), so it is left alone here.Background
server.close()in Node stops accepting and emits'close'only once every tracked connection has ended;net.tsimplements this in_emitCloseIfDrained, which is re-checked as each accepted socket closes.allowHalfOpen): when the peer sends FIN, a Node socket only auto-ends its own side after the readable side emits'end', which requires buffered data to be consumed. A connection nobody reads therefore stays open after the peer hangs up. That is why the stray accepted connection is permanent in 1.4 and why the fix is to not accept it in the first place.listen(2), the kernel completes TCP handshakes on its own and queues the connections until the process callsaccept(2). Closing the listening fd resets whatever is still queued. Bun only accepts when the event loop polls the fd, so whether user code runs before or after that first poll (nextTickvs a timer) decides whether such a connection becomes the server's problem.Repro used
Minimal (server side of vite's probe, port hammered by
curlloops from another shell):main:
accepted connectionx1-4, close callback never fires. 1.3.14: close callback after ~2s (when curl gives up). Node and this branch: closes immediately,accepted=0.Full:
bun create vite app --template react-ts, then startbun --bun node_modules/vite/bin/vite.js dev --port 5173 --host 127.0.0.1whilecurlloops hit the port. main: hangs with an empty log every run; this branch: ready every run.