Conversation
Server.listen() in node:net (and the cluster paths) and node:http deferred the 'listening' and listen 'error' emits with setTimeout(fn, 1). While jest.useFakeTimers() is active that timer goes into the fake timer heap, so the listen callback never runs until the test advances fake time, and the common beforeAll(() => new Promise(r => server.listen(0, r))) hangs. Bun.listen()/Bun.serve() bind, listen and register the socket with the event loop synchronously, so the extra event loop turn the old comment asked for is not needed. Defer with process.nextTick like Node's setupListenHandle does; nextTick is not affected by fake timers.
Contributor
|
Warning Review limit reached
Next review available in: 5 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 (3)
Comment |
Collaborator
Author
|
Status: ready for review. Reproduced on the released 1.4.0 with a |
Collaborator
Author
|
Updated 9:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 9af6c23 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37959That installs a local version of the PR into your bun-37959 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The six setTimeout(..., 1) → process.nextTick conversions are consistent, match Node's setupListenHandle, and the new tests exercise both the 'listening' and EADDRINUSE 'error' paths for net and http. Because it deliberately reverses an explicit "process.nextTick() is not sufficient because it will run before the IO queue" guard in net.ts and changes when 'listening'/'error' fire relative to the I/O poll for every net/http/tls server, a maintainer look to confirm that rationale is indeed stale would be worthwhile.
What was checked:
- All six former
setTimeout(emit*, 1, ...)sites innet.ts(kRealListen,kClusterFauxListen,listen()catch,listenInClusteradopt error) and_http_server.ts(kRealListen,listen()catch) are converted; no stragglers remain. emitListeningNextTickin both files still guards on the handle/server being open, solisten(); close()in the same tick still emits nothing.- The
getTimerCount() === 0assertion in the http test runs beforeawait listening, sosetupConnectionsTracking'ssetInterval(created on'listening') does not falsify it. process.nextTickis already used ~25× in each file, so this follows the local convention.
Extended reasoning...
Overview
This PR converts six setTimeout(fn, 1, ...) deferrals to process.nextTick(fn, ...) in src/js/node/net.ts and src/js/node/_http_server.ts, covering both outcomes of Server.listen(): the 'listening' emit in kRealListen / kClusterFauxListen, and the 'error' emit in the listen() catch blocks and the cluster handle-adopt failure path. It adds a named emitListenErrorNextTick helper in _http_server.ts (replacing an inline arrow, matching the existing style), rewrites the justifying comments, and adds four tests in test/js/bun/test/test-timers.test.ts that assert jest.getTimerCount() === 0 immediately after listen() and that 'listening' / EADDRINUSE 'error' arrive without advancing fake time.
Security risks
None. This is purely event-scheduling within the Node compat layer; no input parsing, auth, crypto, or resource-limit logic is touched.
Level of scrutiny
Medium-high. The code transformation is mechanical, but it changes when 'listening' and listen-'error' fire for every net/tls/http server: from after a 1 ms timer (post-I/O-poll) to the next-tick queue (pre-I/O-poll). The removed comment in net.ts explicitly stated "process.nextTick() is not sufficient because it will run before the IO queue" — the PR argues, with references to us_socket_group_listen and Server::listen, that Bun.listen()/Bun.serve() now bind, listen(2), and register with the poller synchronously, so the guard is stale. That argument reads correctly to me and matches Node's own process.nextTick(emitListeningNT, this), but per the review guidance ("before deleting odd-looking code, git-blame why it was written — it is usually load-bearing"), a maintainer familiar with why the 1 ms timer was originally chosen should sign off.
Other factors
- The PR ran a broad set of net/http/tls/cluster/express suites plus ~135 upstream Node parallel tests and reports parity with main, which is the right validation for an ordering change like this.
process.nextTickis already the dominant deferral mechanism in both files (~25 uses each), and both helper functions were already named*NextTick, so the change aligns with existing conventions rather than introducing a new pattern.- I confirmed the http test's
getTimerCount() === 0assertion is placed synchronously afterlisten()and before the nextTick drains, so theconnectionsCheckingIntervalsetInterval(created inside the'listening'handler) cannot falsify it — the PR description already flags that interval as a separate follow-up. - Tests use
port: 0on127.0.0.1, clean up viatry/finally, and restore real timers on every path.
Collaborator
Author
robobun
added a commit
that referenced
this pull request
Aug 22, 2026
Folds in the http hunk and the fake-timer tests of #37959. http.Server emitted 'listening' and a listen() error from a 1 ms timer. Under jest.useFakeTimers() that timer sits in the fake heap, so listen(0, cb) never calls cb until the test advances fake time. Both emits now use process.nextTick, as node does and as net.Server does since the previous commits. Tests: net and http 'listening' and EADDRINUSE 'error' under fake timers (test-timers.test.ts), http 'listening' and listen() error on the next tick (node-http.test.ts). The net error-order test drops its host argument: with one, node resolves the host through dns.lookup first, which adds a tick.
Collaborator
Author
|
Closing in favor of #40041, which now carries all of this PR.
The fake-timer symptom ( |
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 23, 2026
… a 1 ms timer (#40041) ### Problem - `child_process_ipc_handle.test.ts` > "a handle sent right before the child's disconnect() is still delivered" is red on main (darwin aarch64, builds 103063 and 102999): the parent never gets the `net.Server`. - Cause: `net.Server` emits `'listening'` from a 1 ms timer (`net.ts:3976`). A received server is emitted from that event (`Ipc.ts:58`), so it arrives one I/O poll late: after the next message, with its backlog already accepted. A `close()` from `'listening'` can inherit a connection, which hangs vite's port probe (#39114). - `http.Server` has the same timer (`_http_server.ts:1067`). Under `jest.useFakeTimers()` it never fires, so `listen(0, cb)` never calls `cb` (#37959). http also registered `cb` only after a successful bind, so a `listen(0)` retried from the EADDRINUSE `'error'` handler never called it: vite's port auto-increment hangs (#27406). ### Fix - Both servers emit `'listening'`, and a failed `listen()` its `'error'`, with `process.nextTick`, as node does. `Bun.listen()` and `Bun.serve()` have called `listen(2)` when they return, so the event is not early. A tick runs before the next I/O poll and is not a timer. - http registers the `listen()` callback before the bind and sets `server.listening` as soon as `Bun.serve()` returns, as node and `net.Server` do. - `net.Server.close()` queues a no-op `setImmediate` so the loop lives one more turn, as node's `uv_close()` does. The timer provided that turn by accident. http gets it from the `Bun.serve` stop task. - Verified: eighteen new or pinned tests in five files, fourteen fail on the unfixed build (notes). The net, tls, http, cluster, IPC, third-party and 54 upstream suites match main. Adopts #39114. Includes the http hunk and tests of #37959. Fixes #27406. Supersedes #37802 and #39242. ### Background - Handle passing: `process.send(msg, server)` sends the socket's descriptor with the message. The receiver wraps it in a new `net.Server` and emits `'message'` from its `'listening'`. Later messages wait for the receiver's ack. - Accept backlog: connections wait in the kernel until a holder of the listening socket accepts them. bun accepts the whole backlog on the first readable poll. - Fake timers: `jest.useFakeTimers()` puts every new `setTimeout`, even one made by a built-in module, into a heap that only `jest.advanceTimersByTime()` drains. `process.nextTick` is never diverted. <details><summary>Notes</summary> Tests (thirteen fail on the unfixed build): `node-net-server.test.ts`: `'listening'` before a later `nextTick`, a listen `'error'` before a later `nextTick`, no accept before a `close()` from the `'listening'` handler, `'beforeExit'` re-emitted (passes on main, fails with the nextTick change alone, which is why `close()` holds the loop). `node-http.test.ts`: the same two order tests for http (they also assert `server.listening` right after `listen()`: true after a bind, false after EADDRINUSE), the retry shape of #27406 (the callback runs once after the retry, fails on main), and http and https twins of the `'beforeExit'` test (pass on main too: the 1 ms timer gave the turn by accident, now the task that emits `'close'` gives it). `cluster.test.ts`: a worker's `listen()` callback runs before the worker reports `'listening'` to the primary, for net and http (http fails on main: the callback used to be registered after the notifier). `child_process_ipc_handle.test.ts`: backlog served after a handoff (`acceptedBeforeDelivery: 20, served: 0` on the released bun), a handle before the message sent after it, and the disconnect test pinned to node's order and reporting on `'close'`. `test-timers.test.ts`: `'listening'` and EADDRINUSE `'error'` under fake timers for net and http (`jest.getTimerCount()` is 1 right after `listen()` on main). Earlier shape of this PR: emit the received server right after `listen({ fd })` returns, with `Ipc.ts` the only runtime change. A review pointed out that this patches the consumer around the stale timer, duplicates the open root fix #39114, and diverges from node once that lands (the received server would emit `'listening'` after `'message'`). This PR now carries the root fix. All three IPC tests pass with the nextTick `'listening'` and the unchanged `Ipc.ts`. #27406 ("`http.Server.listen()` hangs when called again after EADDRINUSE") was closed as not reproducible on 1.3.13. It reproduces on 1.4.0 with a dynamic port: `http.Server.listen` passed the callback into `kRealListen`, which registered it as `once('listening')` only after `Bun.serve()` returned. On EADDRINUSE `Bun.serve()` throws first, so the callback was dropped and the retry's `'listening'` had no listener for it. node (`Server.prototype.listen`: `if (cb !== null) this.once('listening', cb)` before `listenInCluster`) and `net.ts:3807` register it before the bind. The review of this PR found it in the hunk being changed; the fix is the same three lines in `Server.prototype.listen`, and `kRealListen` loses its `onListen` argument. In a cluster worker the user callback now runs before the `act: 'listening'` notification to the primary, as in node (node v26.3.0: `callback:online, cluster:listening` for net and http), and `cluster.worker.state` is still `'online'` inside it. `server.listening` on http was set to true only by the deferred emit, so it lagged `address()` by a tick (by 1 ms before this PR). Node's `listening` is a getter on `_handle`, true as soon as `listen()` returns for the host-less form (with a host, both runtimes report false for one tick: node because of `dns.lookup`, bun because the flag was set by the emit; now bun reports true at once in both forms). `kRealListen` sets the flag right after `Bun.serve()` returns; `close()` and `closeAllConnections()` already clear it, and `emitListeningNextTick` only guards on the handle now, as `net.ts` does. #37959 made the same four `net.ts` changes for the fake-timer symptom and also moved the two `_http_server.ts` emits. Its http hunk and its four tests are folded in here (commit "node:http: emit listen() results on the next tick too"), and #37959 is closed in favor of this PR. The http path was checked like net: a server listened from `'beforeExit'` and closed from `'listening'` prints `beforeExit, listening, nextTick, closeCb, close, beforeExit again` on the debug build. Node prints the same order, except that it emits `'close'` before the `close()` callback, a pre-existing difference not touched here. `http.Server.close()` goes through the `Bun.serve` stop, whose all-connections-closed task (`ServerAllConnectionsClosedTask`, an event-loop task, not a microtask) keeps the loop alive for that turn and then emits `'close'`, so http needs no `setImmediate`; the http and https `'beforeExit'` tests pin that turn. Node's `listen(port, host)` resolves `host` through `dns.lookup` before it binds, and `dns.lookup` defers even an IP literal to a `process.nextTick`. So in node, `listen(port, "127.0.0.1")` emits one tick later than `listen(port)`. bun binds an IP literal synchronously in both cases. The order tests therefore call `listen()` without a host, where node v26.3.0 and bun agree tick for tick: `listening, nextTick` and `error:EADDRINUSE, nextTick`. `node-http.test.ts` requires its tests to pass in node. Measurements with the fixture of the disconnect test, 16 at a time: - bun 1.4.0 (unfixed, report on `'exit'`, 200 runs): 175 deliver the server after `'disconnect'`, 13 between the message and `'disconnect'`, 11 have no server at `'exit'` (the CI shape), 1 has node's order. - bun 1.4.0 (unfixed, report on `'close'`, 200 runs): 199 have the wrong order, 1 of them has no server even at `'close'`. - node v26.3.0, 50 runs: all `handle:srv, after-handle, disconnect`. - debug build with this change, report on `'close'`, 150 runs: all `handle:srv, after-handle, disconnect`. Why `'close'`: `fork()` counts the IPC channel as a channel to close (`child_process.ts:1468`), and `#onDisconnect` emits `'disconnect'` one tick before it counts the close (`child_process.ts:1555`). The stderr pipe counts too (`child_process.ts:1258`), so the child's report is complete at `'close'`. On `'exit'` neither is guaranteed: the IPC EOF is a deferred task, so `'disconnect'` can follow `'exit'`. With the fix and the report on `'exit'`, 18 of 100 runs still had no `'disconnect'`. Why the order is send order: once a message with a handle is fully written, the sender moves it to `waiting_for_ack` and writes nothing but an ack or nack until the receiver's ack arrives (`ipc.rs:1487`, `ipc.rs:1550`). So the message behind a handle never shares a read buffer with it. On the receiver, `PosixSocket::on_data` (`ipc.rs:2436`) and `WindowsNamedPipe::on_read` (`ipc.rs:2515`) decode a whole read buffer inside one event-loop scope, and the nextTick queue drains with the microtasks when that outer scope exits (`event_loop.rs:313`), before the next I/O poll and so before the ack'd follow-up message is read. The scope inside the handle arm (`ipc.rs:2169`) is nested and does not drain on its own. With the 1 ms timer, the follow-up message was read by a poll that ran before the timer expired, so it overtook the handle. Why the timer was there: 25097cd (2023-03-18, "[node:net] Fix issue with `listen` callback firing before it's listening") replaced the `process.nextTick` of #2337 with `setTimeout(..., 1)`, with no test and no linked issue, and kept the error path on `nextTick`. The error path moved to the timer in 6baedd2 (tls.Server, #2552) and #31829 copied it for the cluster path. Whatever the symptom was then, the socket is listening when `listen()` returns today: `Bun.listen()` runs `us_socket_group_listen` -> `bsd_create_listen_socket` -> `bind(2)` and `listen(2)` before it returns (`packages/bun-usockets/src/context.c:388`, `bsd.c:1328`, `bsd.c:1067`), and `kRealListen` already depends on that: it reads the bound address with `getsockname` right after `Bun.listen()` returns (`net.ts:3952`) and chmods the unix socket file (`net.ts:3906`). `Bun.serve()` binds and listens in `Server::listen` (`src/runtime/server/mod.rs`) before it returns, and `_http_server.ts` reads `this[serverSymbol].port` right after. The `'listening'` tests and the upstream files are the evidence that nothing needs the delay. #39628 (deferred module init in `node:net`) is not a factor: bun 1.4.0 has #31829 but not #39628 and reproduces the failure. Not changed: the `dgram.Socket` arm of `parseHandle` emits from `bind()`'s callback, which resolves in the microtask drain of the same read. `http.Server`'s `connectionsCheckingInterval` (a 30 s `setInterval` made on `'listening'`) still lands in the fake heap once `'listening'` fires under fake timers; #37987 covers built-in timers under fake timers in general. #37831 (close the descriptor when `listen({ fd })` refuses it) stays independent. #34659 adds a `nextTickListening` flag to `kRealListen` for cluster-adopted descriptors and deflakes the disconnect test by reporting at process exit with `got.sort()`; both hunks become unnecessary with this change. Suites run with the debug build, same results as main: `test/js/node/net` (the 10 `localhost` failures of `node-net.test.ts` fail on the released bun too), `test/js/node/tls` (1 `ECONNREFUSED` test on a `localhost` bind, same on the released bun), `test/js/node/http` (`node-http.test.ts`: 146 pass, the proxy test fails on `localhost` on the released bun too; `node-http-connect.test.ts`: one 5 s spawn timeout under the debug build, same without the http change), `test/js/node/http2`, `test/js/bun/net` (the `localhost` failures, same on the released bun), `cluster.test.ts` (32 pass), `child_process_ipc.test.js`, `child_process_send_cb.test.js`, `child_process_ipc_large_disconnect.test.js`, `spawn.ipc*.test.ts`, `bun-ipc-inherit.test.ts` (55 pass with cluster), `test/js/third_party/express`, `@fastify`, `socket.io` (308 pass, the same 2 timeouts without the http change). Upstream, all pass: `test-net-listen-*` (9), `test-net-server-listen-*` (4), `test-net-server-close*` (3), `test-net-server-unref*` (2), `test-net-listening`, `test-net-pingpong`, `test-process-beforeexit`, `test-child-process-fork-net-server`, `fork-net-socket`, `send-keep-open`, `test-cluster-basic`, `dgram-1`, `dgram-2`, `eaddrinuse`, `listening-port`, `message`, `net-send`, `server-restart-none`, `server-restart-rr`, `shared-handle-bind-error`, `test-cluster-http-pipe`, `test-http-listening`, `test-http-server-close-all`, `test-http-server-close-destroy-timeout`, `test-http-server-close-idle`, `test-http-server-close-idle-wait-response`, `test-http-server-consumed-timeout`, `test-http-server-options-incoming-message`, `test-http-server-options-server-response`, `test-https-server-close-all`, `test-https-server-close-destroy-timeout`, `test-https-server-close-idle`, `test-http-bind-twice`, `test-http-server-stale-close`, `test-http-server-keepalive-end`, `test-http-server-multiheaders`, `test-http-server-keep-alive-timeout`. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/net/node-net-server.test.ts, test/js/node/cluster.test.ts, test/js/node/child_process/child_process_ipc_handle.test.ts <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
jest.useFakeTimers()active,http.createServer().listen(0, cb)never callscbandserver.listeningstaysfalseuntil the test advances fake time;jest.getTimerCount()reports1right afterlisten(). The usualbeforeAll(() => new Promise(r => server.listen(0, r)))hangs until the hook timeout.net.createServer()/tls.createServer()behave the same way, and so does the listen'error'path (EADDRINUSE is never emitted).setTimeout(emitListeningNextTick, 1, ...)insrc/js/node/_http_server.ts(kRealListen) andsrc/js/node/net.ts(kRealListen,kClusterFauxListen), andsetTimeout(..., 1)for the'error'emit in bothlisten()catch blocks and the cluster adopt path. Fake timers work at the timer heap level, so asetTimeoutmade by a built-in module is captured like one made by the test.net.tscomment justifying the timer ("the server may not actually be listening yet, nextTick runs before the IO queue") dates from 2023 and no longer holds:Bun.listen()andBun.serve()create, bind,listen(2)and register the socket with epoll/kqueue/libuv before they return (us_socket_group_listeninpackages/bun-usockets/src/context.c,Server::listeninsrc/runtime/server/mod.rs), andaddress()/portread the live socket synchronously.net.tsalready relies on this a few lines earlier (the unix socket chmod afterBun.listen).Fix
process.nextTick, which is what Node'ssetupListenHandledoes (process.nextTick(emitListeningNT, this)/process.nextTick(emitErrorNT, this, ex)) and what the helper functions here were already named after.process.nextTickis not a timer, so fake timers do not affect it.listen(); close()in the same tick still emits nothing, as in Node), listeners attached synchronously afterlisten()still see'listening'/'error', andlisten()no longer costs a 1 ms timer per server.test/js/bun/test/test-timers.test.ts(netandhttp,'listening'and EADDRINUSE'error'); each fails in a few ms on main (getTimerCount()is 1) and passes with the fix.test/js/node/net/node-net-server.test.ts,server.spec.ts,node-net.test.ts,test/js/node/http/node-http.test.ts,node-http-server-timeouts.test.ts,node-http-with-ws.test.ts,test/js/node/cluster.test.ts,test/js/node/tls/node-tls-server.test.ts,test/js/third_party/express, and ~135 upstreamtest-net-*/test-http*-server-*/test-cluster-*/test-*-bind-twice/test-*-eaddrinusefiles. The only failures are the same ones main has in this container (tests that listen onlocalhostwhile/etc/hostslists::1first, tests that need the public internet, and a 500 ms fetch budget on a debug build).'listening'fires under fake timers,http.Server'sconnectionsCheckingInterval(a 30 ssetIntervalcreated insetupConnectionsTracking) now lands in the fake heap, sogetTimerCount()is 1 while such a server is open. That is the same "built-in module uses a JS timer" shape as thechild_processtimeout noted in bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 and needs a way for built-ins to schedule real timers; bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 covers the native-side timers and does not touch these JS sites, so the two do not overlap.Background
bun:testfake timers:jest.useFakeTimers()makestimer::All::insertroute every new timer whose tag allows it (including everysetTimeout/setInterval/setImmediate,TimeoutObject/ImmediateObject) into a separate heap that onlyjest.advanceTimersByTime()and friends drain. The built-in modules insrc/js/run against the same globals as user code, so asetTimeoutinsidenode:netis indistinguishable from one in the test.process.nextTickcallbacks and promise jobs are run by the event loop directly and are never diverted.kRealListenis the step ofServer.listen()that actually creates the native listener (Bun.listenfornode:net,Bun.servefornode:http);kClusterFauxListenis the cluster worker variant that wraps a handle handed over by the primary instead of binding itself. Both end by announcing the result to JS, which is the part this PR changes.'listening'(and a failed bind's'error') to the next tick rather than emitting synchronously so that callers can attach listeners afterlisten()returns; it has never waited for an event loop turn.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file