node:http: skip TCP_DEFER_ACCEPT so server.setTimeout fires on time for silent clients - #36074
node:http: skip TCP_DEFER_ACCEPT so server.setTimeout fires on time for silent clients#36074robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that skips TCP_DEFER_ACCEPT only for node:http compat contexts so 'connection' and server.setTimeout are measured from TCP accept, matching Node.
What was reviewed:
- Confirmed
enableNodeHttpCompat()runs beforelisten()(viaassignOnNodeJSCompatin NodeHTTP.cpp), soisNodeHttp()is accurate at the gate. Bun.servekeeps the defer-accept optimisation;listen_unixnever set the flag so no sibling change needed.- The
resume_()→resume()renames are the mechanical build fix already on main (5008c2c). - Test bounds (< 900 ms / < 1250 ms) are categorical vs the 1 s kernel defer window, not a race, and every failure path is wired to the awaited
close.
Extended reasoning...
Overview
The functional change is a 3-line gate in packages/bun-uws/src/HttpContext.h::listen(): LIBUS_LISTEN_DEFER_ACCEPT is now OR'd into options only when !isNodeHttp(). The remaining Rust hunks (RequestContext.rs, streams.rs) are the resume_() → resume() rename that already landed on main as 5008c2c and is carried here so the branch compiles. A new regression test in test/js/node/http/node-http-server-timeouts.test.ts connects without writing and asserts 'connection' fires promptly and the 500 ms inactivity timeout lands under 1250 ms.
Security risks
None. This only decides whether TCP_DEFER_ACCEPT is set on a listen socket; no new input parsing, allocation, or trust boundary. If anything it slightly narrows a slow-loris variant for node:http (silent-connect clients now start their inactivity timer immediately instead of after the kernel's 1 s defer).
Level of scrutiny
Low-to-medium. The mechanism is well explained and verifiable: TCP_DEFER_ACCEPT on Linux holds back a data-less connection for the configured timeout, delaying uWS onOpen and the filter callback that emits node:http's 'connection'. The gate uses the existing isNodeHttp() predicate (vtable identity), which multiple in-tree comments already document as being set before listen(); I traced the call chain through assignOnNodeJSCompat → app->enableNodeHttpCompat() to confirm. Bun.serve is unaffected, so this respects the "don't change a Bun-native default to fix Node compat" rule. listen_unix never set the flag, so no sibling site was missed.
Other factors
The test uses timing thresholds, but they are structurally sound: the defer window is a hard 1 s floor, so connectionAt < 900 vs the ~1000 ms observed on main is a categorical distinction rather than a scheduler race, and the comment says as much. The test awaits the socket 'close' event (with 'error' wired to a no-op) rather than sleeping, and cleans up in finally. The resume_ → resume hunks will drop out on rebase since 5008c2c is already on main.
…or silent clients The HttpContext listener unconditionally set LIBUS_LISTEN_DEFER_ACCEPT (TCP_DEFER_ACCEPT with a 1 second timeout on Linux). For node:http compat that made the server see a silent connection roughly a second late: the 'connection' event fired after the kernel's defer window expired, and the socket.setTimeout armed in the NodeHTTPServerSocket constructor only started counting from there. A server configured with server.setTimeout(500) would close an idle, never-writing client after ~1.5s instead of ~0.5s (Node.js fires within a few ms of the configured value). Skip the defer-accept flag when the context is in node:http compat mode so accept() (and therefore 'connection' and the per-socket inactivity timer) runs at TCP accept time like Node.js. Bun.serve keeps the optimisation.
fb5edda to
7529c3f
Compare
There was a problem hiding this comment.
LGTM — gates TCP_DEFER_ACCEPT on !isNodeHttp() so node:http's 'connection' event and server.setTimeout are measured from TCP accept, matching Node; Bun.serve keeps the optimisation.
What was reviewed:
- Confirmed
enableNodeHttpCompat()swaps the vtable beforelisten()(asserted viahead_listen_sockets == nullptr), soisNodeHttp()is accurate at this call site — same predicate already used forsocketExtSize()right below. bsd_set_defer_acceptsets a 1 sTCP_DEFER_ACCEPTon Linux, so the test's <900 ms / <1250 ms bounds distinguish immediate accept from deferred accept with margin for ASAN.listen_unix()never OR'd the flag in, so no sibling fix needed there.
Extended reasoning...
Overview
Two-file change: packages/bun-uws/src/HttpContext.h makes HttpContext::listen() skip LIBUS_LISTEN_DEFER_ACCEPT when the context is in node:http compat mode, and test/js/node/http/node-http-server-timeouts.test.ts gains a test that connects without writing and asserts the server's 'connection' event fires promptly (<900 ms) and a 500 ms setTimeout fires within 1250 ms.
Security risks
None. This drops a Linux-only accept-time optimisation for the node:http path; it does not touch auth, TLS, parsing, or any untrusted-input handling. Removing deferred accept cannot introduce a new attack surface — Node.js itself never used it.
Level of scrutiny
Medium. The listen path is production-critical, but the edit is a 3-line conditional on an existing predicate (isNodeHttp()) that is already load-bearing for socketExtSize() in the same function group. The vtable swap in enableNodeHttpCompat() is asserted to happen before any listen socket exists, so the predicate is guaranteed correct at this point. Bun.serve is unaffected (still gets LIBUS_LISTEN_DEFER_ACCEPT), so the perf tradeoff is scoped to node:http compat, where matching Node's accept semantics is required.
Other factors
- Verified
bsd_set_defer_acceptin bsd.c uses a 1 sTCP_DEFER_ACCEPTtimeout, which matches the PR's root-cause analysis and justifies the test's timing bounds (900 ms is comfortably below the 1 s defer window; 1250 ms gives ~750 ms slack over the 500 ms setTimeout for debug+ASAN). - Checked for sibling sites:
listen_unix()never set the flag, and the onlyLIBUS_LISTEN_DEFER_ACCEPTproducer in the tree is this line, so no other call site needs the same guard. - The test's
t0is aletinitialised beforenet.connectand reassigned on client'connect'; the server-side'connection'closure reads it after both are on the same event-loop turn, so no TDZ and negligible skew on localhost. - On macOS/Windows the test would pass with or without the fix (no
TCP_DEFER_ACCEPT), but it still asserts the correct Node-compatible contract there. - PR evidence shows the new test fails on main (connection at ~1000 ms, timeout at ~1500 ms) and passes on both debug+ASAN and release with the fix.
|
CI status: the diff is green. The added test ( The remaining red is unrelated to this change:
Ready for review. |
|
Updated 5:53 AM PT - Jul 27th, 2026
❌ @robobun, your commit 7529c3f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36074That installs a local version of the PR into your bun-36074 --bun |
|
Independent confirmation of the On current main (8a1cd8d), with a client that connects to a
Node v26.3.0 emits at ~1 to 2 ms in all three cases.
The diff still applies cleanly on main. Beyond |
2 participants
What
server.setTimeout(N)fires roughly a second late for a client that connects and never sends. The'connection'event is late by the same amount.Repro
Node v26:
{"t500":504,"t1500":1502,"t2500":2503,"t3500":3500}Bun (before):
{"t500":1811,"t1500":2539,"t2500":3540,"t3500":4564}Bun (after):
{"t500":586,"t1500":1530,"t2500":2529,"t3500":3530}(debug+ASAN overhead)Cause
HttpContext::listenunconditionally setLIBUS_LISTEN_DEFER_ACCEPT, which appliesTCP_DEFER_ACCEPTwith a 1 second timeout on Linux. When a client connects and never writes, the kernel holds the connection back fromaccept()for that full second. uWS'sonOpen(and the filter callback that drives node:http'sonServerConnection) only runs after the defer window expires, so theNodeHTTPServerSocketis constructed,'connection'is emitted, andsocket.setTimeout(server.timeout)is armed roughly a second late.headersTimeout/requestTimeoutlook precise in the same scenario because those tests send bytes, and any data short-circuits the defer.Fix
Skip
LIBUS_LISTEN_DEFER_ACCEPTwhen the context is in node:http compat mode (isNodeHttp()is already set beforelisten()). Node.js does not use deferred accept; the'connection'event and the per-socket inactivity timer are measured from TCP accept.Bun.servekeeps the optimisation.Verification
New test in
test/js/node/http/node-http-server-timeouts.test.tsconnects without writing and asserts'connection'is seen in under 900 ms and the 500 ms timeout fires in under 1250 ms. Fails on main (connection seen at ~1000 ms, timeout at ~1500 ms), passes with the fix.First commit is the same
resume_()→resume()build fix as #36072 so this branch compiles; drop it once that lands.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file
root cause · written by the author bot
The listen socket was unconditionally setting TCP_DEFER_ACCEPT, so on Linux a client that connects without writing is not handed to userspace for roughly one second, delaying the 'connection' event and the start of the idle timer by that amount. The fix stops passing the defer-accept option when the HttpContext is running in node:http compat mode, so connections are accepted at TCP establishment and server.setTimeout measures from the correct origin. Bun.serve retains the defer-accept optimisation since it does not expose these Node semantics.