Skip to content

node:http: let server 'timeout' listeners veto the default socket destroy - #33735

Closed
robobun wants to merge 11 commits into
mainfrom
claude/a00c4899-http-server-settimeout-veto
Closed

robobun wants to merge 11 commits into
mainfrom
claude/a00c4899-http-server-settimeout-veto

Conversation

@robobun

@robobun robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Node's contract for server.setTimeout(msecs) is that once a 'timeout' listener is installed on the server, req, or res, the runtime does not destroy the socket when the timeout fires; the listener decides. Bun fired the event and tore the connection down anyway, and an idle keep-alive connection reaped by the server timeout never emitted 'timeout' at all.

Repro

import http from "node:http";
import net from "node:net";

const srv = http.createServer((q, r) => { q.resume(); });
srv.listen(0, "127.0.0.1", () => {
  srv.setTimeout(600);
  srv.on("timeout", () => {}); // records only, never destroys
  const s = net.connect(srv.address().port, "127.0.0.1");
  s.on("connect", () => s.write("POST / HTTP/1.1\r\nHost: a\r\nContent-Length: 900\r\n\r\nab"));
  s.on("close", () => console.log("server closed the connection"));
});

Node keeps the connection open (the listener vetoes the destroy). Bun closed it ~4s after connect regardless.

Cause

Server.prototype.setTimeout called setServerIdleTimeout(server, ceil(ms/1000)), which sets the uWS per-connection idle timeout. When that fires, HttpContext::onTimeout runs the JS callback and then unconditionally close()s the socket:

if (httpResponseData->onTimeout) {
    httpResponseData->onTimeout((HttpResponse<SSL> *)s, httpResponseData->userData);
}
return asyncSocket->close();

So onNodeHTTPServerSocketTimeout's if (!reqTimeout && !resTimeout && !serverTimeout) this.destroy() was dead code: by the time it ran, the native close was already committed. For an idle keep-alive socket there is no NodeHTTPResponse attached, so no JS callback fired at all; the socket was closed silently.

Fix

Drive the server timeout from an unref'd JS timer per connection, like Node.js. Server.prototype.setTimeout now only stores this.timeout and registers the callback. Each request on a connection arms socket.setTimeout(server.timeout); the timer fires socket._onTimeout() -> emit('timeout') -> the existing onNodeHTTPServerSocketTimeout veto logic, which now actually decides. Activity (new request, body data, tunnel data/drain, response finish) refreshes the timer via _unrefTimer(), and the timer is cleared on close.

The native idle-timeout path is no longer used for node:http (the server already passes idleTimeout: 0 to Bun.serve).

Verification

$ bun bd test test/js/node/http/node-http-server-setTimeout.test.ts
 6 pass
$ USE_SYSTEM_BUN=1 bun test test/js/node/http/node-http-server-setTimeout.test.ts
 1 pass
 5 fail

The one passing case on the old build is "with no listener, the socket is destroyed on timeout", which both implementations satisfy.

…troy

Server.prototype.setTimeout was arming the native uWS per-connection idle
timeout (setServerIdleTimeout), which closes the socket unconditionally in
HttpContext.h's onTimeout handler after running the JS callback. The
socketOnTimeout-style veto logic (emit on req/res/server; destroy only if
no listener handled it) ran, but the native close was already committed,
so a 'timeout' listener could observe the event but not keep the
connection alive. Idle keep-alive connections were reaped with no
'timeout' event at all, since their NodeHTTPResponse was gone by then.

Drive server.setTimeout from an unref'd JS timer per connection instead,
like Node.js: Server.prototype.setTimeout stores server.timeout and each
request arms/refreshes socket.setTimeout(server.timeout). The timer
fires socket._onTimeout -> emit('timeout') -> onNodeHTTPServerSocketTimeout,
which now makes the actual destroy decision. Activity (new request,
body data, tunnel data/drain, response finish) refreshes the timer.
@coderabbitai

coderabbitai Bot commented Jul 8, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR removes the server idle-timeout API and shifts HTTP timeout handling to per-socket timers. It also updates incoming, response, runtime, and test paths to match the new timeout listener behavior.

Changes

Idle-timeout removal and socket timeout handling

Layer / File(s) Summary
Native API removal
src/runtime/server/server_body.rs, src/runtime/server/mod.rs, src/jsc/bindings/NodeHTTP.cpp
Removes the C-ABI idle-timeout entrypoint, Rust dispatcher helper, NewServer::set_idle_timeout, and the JSC/internal binding wiring for setServerIdleTimeout.
Internal HTTP exports
src/js/internal/http.ts
Removes setServerIdleTimeout, setRequestTimeout, and kDeferredTimeouts, and adds kBodyChunks, kClearTimeout, and kCloseCallback to the export set.
Server timeout setup and dispatch
src/js/node/_http_server.ts, src/js/node/_http_incoming.ts
Initializes server.timeout, rewrites Server.prototype.setTimeout, applies timeout values to sockets during request dispatch, and forwards IncomingMessage.setTimeout to the socket timeout path.
Per-socket timeout lifecycle
src/js/node/_http_server.ts
Adds managed socket timeout state, refresh/unref behavior, close cleanup, buffered-response timeout suppression, and single-timer setTimeout(msecs, callback) handling.
Response and incoming data timer refresh
src/js/node/_http_incoming.ts, src/js/node/_http_server.ts
Unrefs timers during incoming data handling, finish handling, and response write/send/drain paths.
Timeout behavior tests
test/js/node/http/node-http-server-setTimeout.test.ts, test/js/bun/test/parallel/test-http-should-emit-timeout-event.ts, test/js/bun/test/parallel/test-http-should-emit-timeout-event-when-using-server-setTimeout.ts
Adds timeout behavior coverage for server/request listeners, idle and stalled connections, streaming writes, and req.setTimeout(0), and updates listener tests to destroy sockets explicitly.
Runtime timeout callback removal
src/runtime/server/NodeHTTPResponse.rs, src/runtime/webcore/Request.rs
Removes the request and response timeout-setting callbacks and their related Rust helper methods.
Suggested reviewers: cirospaciari, alii
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main behavioral change to node:http timeout handling.
Description check ✅ Passed The description covers the PR purpose and verification, though it uses Repro/Cause/Fix/Verification instead of the exact template headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@github-actions github-actions Bot added the claude label Jul 8, 2026
@robobun

robobun commented Jul 8, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:13 AM PT - Jul 8th, 2026

❌ @robobun, your commit b7f981c has 2 failures in Build #70422 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33735

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

bun-33735 --bun

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. Fastify timeout settings not respected when using Bun runtime with NestJS and Fastify adapter #17287 - Fastify timeout settings not respected: Server.prototype.setTimeout now properly stores and applies the timeout per-connection via JS timers
  2. IncomingMessage socket unref doesn't work #23776 - IncomingMessage socket unref doesn't work: _unrefTimer() is no longer a no-op and the per-socket timer is properly unref()'d

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #17287
Fixes #23776

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. node:http: wire server.timeout to the idle timeout #33733 - Also rewrites Server.prototype.setTimeout and wires server.timeout to the timeout mechanism, fixing the same broken timeout storage/routing
  2. node:http: wire server socket/response setTimeout to the native inactivity timer #33732 - Also replaces the NodeHTTPServerSocket.setTimeout stub with a full implementation including timer fields and callback registration

🤖 Generated with Claude Code

@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Re the duplicate-PR bot: #33732 and #33733 touch the same lines but address different bugs (the socket.setTimeout stub and the missing server.timeout property respectively). Both still route through the native uWS idle timeout, whose HttpContext::onTimeout unconditionally close()s the socket after the callback returns, so neither lets a 'timeout' listener keep the connection alive, and neither emits 'timeout' for an idle keep-alive socket (there is no NodeHTTPResponse attached between requests for the native callback to reach).

This PR drives the timeout from an unref'd JS timer per connection instead, which is what makes the existing veto logic in onNodeHTTPServerSocketTimeout actually decide. It happens to also implement socket.setTimeout and server.timeout along the way, so whichever lands first the other two will need rebasing.

Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/internal/http.ts
Comment thread src/js/node/_http_server.ts Outdated
robobun added 2 commits July 8, 2026 05:22
…etServerIdleTimeout binding

Response body writes (ServerResponse.write/_send and the native drain
callback) now refresh the socket's inactivity timer so server.setTimeout(N)
without a listener no longer destroys an actively-streaming response.
Fix _onTimeout to check the property that actually exists on the native
NodeHTTPResponse (bufferedAmount, not writableLength).

Don't clear a user-set per-socket timeout on the next keep-alive request
when server.timeout is 0: re-arm only when server.timeout is set, otherwise
just refresh any existing timer.

Remove the now-unused jsHTTPSetServerIdleTimeout binding and its Rust-side
Server__setIdleTimeout / set_idle_timeout implementation.
Comment thread src/js/node/_http_server.ts
Comment thread src/js/node/_http_server.ts
Comment thread src/js/node/_http_server.ts Outdated
robobun added 2 commits July 8, 2026 05:51
…e CONNECT; refresh on raw socket _write

IncomingMessage.prototype.setTimeout now delegates to socket.setTimeout so
all four entry points (server/socket/req/res.setTimeout) share the one JS
inactivity timer and req.setTimeout(0) clears what server.setTimeout armed.

Move the per-request arm above the CONNECT early-return so server.timeout
applies to CONNECT tunnels too, and refresh the timer in
NodeHTTPServerSocket._write so raw socket writes (CONNECT/upgrade handlers)
count as activity like Node.js's net.Socket._writeGeneric.
server.setTimeout and req.setTimeout listeners now veto the default
socket destroy (Node.js semantics), so these tests' callbacks must
destroy the socket themselves for the server to close cleanly. The
previous versions relied on the native idle timeout force-closing
the connection regardless.

@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.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 src/js/node/_http_server.ts:1187-1190 — This guard suppresses 'timeout' whenever bufferedAmount > 0 and re-arms via _unrefTimer(), so a client that stops reading after triggering a large buffered write causes the timer to loop _onTimeout → bufferedAmount>0 → refresh forever and the connection is never reaped. Node's net.Socket._onTimeout compares the current writeQueueSize against this[kLastWriteQueueSize] and only suppresses when they differ (progress); a stalled write falls through and emits 'timeout'. Track the last-observed bufferedAmount and only suppress when it has changed since the previous check.

    Extended reasoning...

    What the bug is

    The 36e1b22 fix changed this guard from response.writableLength (always undefined, so dead) to response.bufferedAmount and added the this._unrefTimer() re-arm. That made the presence-only check live: whenever there is any buffered outbound data on the native NodeHTTPResponse, _onTimeout() refreshes #timeoutTimer and returns without emitting. It never checks whether the buffered amount has changed since the last fire.

    Node.js's net.Socket.prototype._onTimeout (lib/net.js) implements a progress check, not a presence check: it compares this._handle.writeQueueSize against this[kLastWriteQueueSize] (cached at the last write and at the last timeout) and suppresses the timeout only when the two differ — i.e. bytes have drained since the last observation. When they're equal (a genuinely stalled write), it falls through, updates kLastWriteQueueSize, and emits 'timeout'. Bun has no equivalent last-value tracking on NodeHTTPServerSocket.

    The specific code path that triggers it

    NodeHTTPResponse.get_buffered_amount() (src/runtime/server/NodeHTTPResponse.rs:654) returns the uWS backpressure buffer size unless REQUEST_HAS_COMPLETED or SOCKET_CLOSED is set — neither is set for a live request whose response write is stalled (client stopped reading, TCP receive window closed). So bufferedAmount stays > 0 across every timer fire.

    The _unrefTimer() calls added elsewhere in 36e1b22 don't help: allowWritesToContinue only fires on native drain (which never happens when nothing is draining), and the ServerResponse.write/_send calls only fire when the handler calls write() again (it isn't). The socket's #onDrain is only wired for CONNECT/upgrade tunnels. Nothing else refreshes or clears the timer.

    Step-by-step proof

    const srv = http.createServer((req, res) => {
      res.writeHead(200);
      res.write(Buffer.alloc(4 * 1024 * 1024));  // exceeds socket send buffer
    });
    srv.setTimeout(5000);   // no 'timeout' listener → default destroy path
    1. Client sends the request, receives the headers, then stops calling read() (TCP receive window closes). The 4 MB write partially flushes; the remainder sits in the uWS backpressure buffer, so response.bufferedAmount > 0.
    2. At t≈5 s #timeoutTimer fires → _onTimeout() → bufferedAmount > 0 → _unrefTimer() → #timeoutTimer.refresh() → return. 'timeout' is not emitted.
    3. At t≈10 s, 15 s, …: same. Loop forever.
    4. onNodeHTTPServerSocketTimeout never runs, this.destroy() never runs, the socket and its multi-MB buffer are held indefinitely.

    In Node.js the same scenario emits 'timeout' and destroys the socket at t≈5 s: on the first _onTimeout, writeQueueSize === kLastWriteQueueSize (nothing drained since the write cached it) → the guard falls through.

    Why this is a regression

    Before this PR, server.setTimeout(N) set config.idle_timeout and each request called resp.timeout(idle_timeout) on the native uWS response. Under a fully-stalled write, uWS's on_writable never fires, so resetTimeout() is never called, and the native onTimeout eventually closes the socket. After this PR, node:http passes idleTimeout: 0 to Bun.serve and relies solely on the JS timer — which now loops forever under the same conditions. So a slow-reader client that lets its TCP window fill after triggering a large response now defeats server.setTimeout entirely (a slowloris-adjacent resource-hold).

    How to fix

    Track the last-observed buffered amount on the socket and only suppress when it has changed since the previous check, mirroring Node's kLastWriteQueueSize:

    #lastBufferedAmount = 0;
    _onTimeout() {
      const response = this[kHandle]?.response;
      const buffered = response ? response.bufferedAmount : 0;
      if (buffered > 0 && buffered !== this.#lastBufferedAmount) {
        this.#lastBufferedAmount = buffered;
        this._unrefTimer();
        return;
      }
      this.#lastBufferedAmount = buffered;
      this.emit('timeout');
    }

    and update #lastBufferedAmount from the write paths (ServerResponse.write/_send/allowWritesToContinue) so an actively-writing response also counts as progress. Alternatively, drop the guard entirely and rely on the write-time _unrefTimer() calls — Node's guard exists because kernel-level write completion doesn't call back into JS; if allowWritesToContinue reliably fires on every partial drain, the guard may be unnecessary.

  • 🟡 src/js/node/_http_server.ts:713-715 — The socket.setTimeout(server.timeout) arm at line 714 runs after server.emit('connection', socket) at line 710, so a user 'connection' listener that calls socket.setTimeout(N) to override the server default is immediately clobbered when server.timeout > 0. In Node.js, connectionListenerInternal (registered as the first 'connection' listener) arms socket.setTimeout(server.timeout) before any user listener runs, so user listeners can override it. Combined with the CONNECT-placement fix already suggested at this line: move the arm to right after the socket is created (above the if (method === "CONNECT") block and before emit('connection')), gated on isSocketNew, and only _unrefTimer() on subsequent requests.

    Extended reasoning...

    What the bug is

    In onNodeHTTPRequest, the server-timeout arm

    const serverTimeoutMs = server.timeout;
    if (serverTimeoutMs) socket.setTimeout(serverTimeoutMs);
    else socket._unrefTimer();

    runs at lines 713-715 — after server.emit('connection', socket) at line 710. When server.timeout > 0, a user 'connection' listener that calls socket.setTimeout(N) to override the server default (a documented Node.js pattern) has its value immediately overwritten by socket.setTimeout(server.timeout) on the very next line.

    Additionally, because this arm is not gated on isSocketNew, every subsequent keep-alive request re-clobbers a user's per-socket timeout back to server.timeout — so the user cannot override server.timeout per-connection at all when server.timeout > 0.

    Node.js behaviour

    In Node.js, the http.Server constructor registers connectionListener as the first 'connection' listener (via net.Server.call(this, opts, connectionListener)). connectionListenerInternal does if (server.timeout) socket.setTimeout(server.timeout) there, before any user-added 'connection' listener runs. So user listeners run afterwards and can override the socket timeout. Subsequent requests on the same connection only refresh via socket._unrefTimer() in parserOnIncoming; they never re-apply server.timeout.

    Step-by-step proof

    const srv = http.createServer((req, res) => res.end('ok'));
    srv.setTimeout(5000);
    srv.on('connection', s => s.setTimeout(30000));  // user wants 30s for this connection
    1. Client connects and sends a request. onNodeHTTPRequest runs with isSocketNew = true.
    2. Line 710: server.emit('connection', socket) — user listener runs, calls socket.setTimeout(30000). Per the new NodeHTTPServerSocket.setTimeout (lines 1330-1342), #timeoutTimer is armed to 30s and socket.timeout = 30000.
    3. Line 713-714: serverTimeoutMs = 5000 (truthy) → socket.setTimeout(5000). This clears the 30s timer and arms a fresh 5s timer.
    4. The user's override is lost; the socket times out at 5s instead of 30s.
    5. On the next keep-alive request, step 3 runs again (isSocketNew = false doesn't gate it), so even a res.setTimeout(30000) set inside request N is clobbered back to 5s at the start of request N+1.

    In Node.js, step 2's user listener runs after Node's own arm, so socket.timeout stays at 30000; and step 5 never happens because Node only _unrefTimer()s on subsequent requests.

    Why this is distinct from earlier comments

    The (now-resolved) comment #3 covered the server.timeout === 0 case — the fix in 36e1b22 chose the else socket._unrefTimer() branch of #3's suggestion rather than gating on isSocketNew, so the serverTimeoutMs > 0 branch still unconditionally re-arms and does so after emit('connection'). Comment #6 is about the arm being unreachable for CONNECT. Neither noted the ordering relative to emit('connection') for the first request, which is the primary observation here.

    Impact

    Node-compat gap in the very feature this PR adds — not a regression vs pre-PR Bun (NodeHTTPServerSocket.setTimeout was previously a no-op stub, so a user's per-connection override never worked anyway). Narrow trigger: server.timeout > 0 and the user overrides per-connection via a 'connection' listener or res.setTimeout.

    Fix

    Move the arm to right after the socket is created/reused — above the if (method === "CONNECT") block and before server.emit('connection', socket) — and gate it on isSocketNew, matching Node's connectionListenerInternal:

    if (!socket) {
      socket = new NodeHTTPServerSocket(server, socketHandle, !!tls);
    }
    if (isSocketNew) {
      const serverTimeoutMs = server.timeout;
      if (serverTimeoutMs) socket.setTimeout(serverTimeoutMs);
    } else {
      socket._unrefTimer();
    }

    This also folds in the CONNECT-placement fix suggested in the other open comment on this line.

  • 🟡 src/js/node/_http_incoming.ts:381 — Read-side sibling of the write-refresh gaps flagged at lines 1083/934/714: this _unrefTimer() only fires once handle.ondata is wired in IncomingMessage._read (i.e. after the user starts consuming). onNodeHTTPRequest calls handle.pause() before emitting 'request', and while paused, incoming body bytes are buffered natively in on_buffer_request_body_while_paused (NodeHTTPResponse.rs) with no JS callback — so an actively-uploading client does not refresh #timeoutTimer until the handler starts reading. On non-Windows do_pause() also OS-pauses the socket so little data arrives anyway; on Windows it does not, making this a genuine regression there vs the pre-PR native idle timeout (which reset on socket-layer reads via HttpContext.h:384). Consider also refreshing in onIncomingMessageResumeNodeHTTPResponse / the drainRequestBody path in _read, which push the initial buffered chunk without calling _unrefTimer.

    Extended reasoning...

    What the bug is

    This is the read-side sibling of the write-side "activity must refresh the JS idle timer" gaps already flagged on this PR (open comments at _http_server.ts:1083, :934, :714). The PR adds this.socket?._unrefTimer?.() in onDataIncomingMessage (_http_incoming.ts:381), but that callback is only reachable once handle.ondata = onDataIncomingMessage.bind(this) is assigned — which happens in IncomingMessage.prototype._read (_http_incoming.ts:364-367), i.e. only after the user starts consuming the body via .on('data') / .pipe() / .resume().

    In onNodeHTTPRequest, when hasBody is true, handle.pause() is called unconditionally before the 'request' event is emitted. While paused, incoming body chunks are delivered to the native shim on_buffer_paused_shim → on_buffer_request_body_while_paused (NodeHTTPResponse.rs:1220-1234), which just appends to buffered_request_body_data_during_pause and sets a flag — no JS callback is invoked. So body bytes arriving during the paused window never call _unrefTimer(), and #timeoutTimer counts down as if the connection were idle.

    Even after the user starts consuming, the initial buffered chunk is delivered via handle.resume() (in onIncomingMessageResumeNodeHTTPResponse) or handle.drainRequestBody() (in _read), both of which this.push(...) directly without calling _unrefTimer(). Only subsequent chunks routed through onDataIncomingMessage refresh the timer.

    Step-by-step proof

    const srv = http.createServer(async (req, res) => {
      await someAsyncAuth(req.headers);   // takes 2s; body not yet consumed
      req.pipe(fs.createWriteStream('/tmp/upload'));
    });
    srv.setTimeout(1000);   // no 'timeout' listener → default destroy
    1. Client sends POST / with a large body, streaming continuously.
    2. onNodeHTTPRequest runs: handle.pause() is called (hasBody true). Then server.timeout === 1000 → socket.setTimeout(1000) arms #timeoutTimer. Then server.emit('request', ...).
    3. The handler awaits async auth. Meanwhile the client keeps sending body bytes. Each chunk reaches on_buffer_request_body_while_paused (NodeHTTPResponse.rs), which appends to the native buffer and returns — no JS callback → no _unrefTimer().
    4. At t≈1000ms #timeoutTimer fires _onTimeout(). response.bufferedAmount is 0 (no outgoing writes yet), so the guard is skipped. this.emit('timeout') → onNodeHTTPServerSocketTimeout: req.complete is false but req has no 'timeout' listener → reqTimeout = false; no res/server listener either → this.destroy(). The actively-uploading connection is torn down.

    Why existing safeguards don't help

    • onDataIncomingMessage's new _unrefTimer() is unreachable while paused: handle.ondata is only assigned in _read, which requires the user to start consuming.
    • The socket's own #onData (which also refreshes) is only wired via kEnableStreaming(true) for CONNECT/upgrade tunnels; normal requests call kEnableStreaming(false) which sets handle.ondata = undefined on the socket handle.
    • _onTimeout's bufferedAmount > 0 guard checks the outgoing response buffer, not the inbound paused-body buffer.

    Regression scope

    This is primarily a Windows regression. do_pause() at NodeHTTPResponse.rs:1019-1022 is gated #[cfg(not(windows))] for the pause_socket() call:

    • Non-Windows: the OS socket is paused, so after the kernel/TCP buffers fill no further data arrives to buffer natively. Both pre-PR (native uWS timer) and post-PR (JS timer) see no read activity and would time out similarly — no behavior change.
    • Windows: the OS socket is not paused, so uWS keeps delivering data into on_buffer_paused_shim. Pre-PR, the native uWS timer at HttpContext.h:382-386 applied a throughput-based resetTimeout() on incoming body data (since inStream is set), keeping a fast uploader alive. Post-PR, the JS timer sees none of it — a genuine regression.

    The Node.js baseline also refreshes on socket reads: net's onStreamRead calls self._unrefTimer() on every incoming chunk before the HTTP parser sees it. Node does eventually pause the socket once IncomingMessage's highWaterMark (~16KB) fills via readStop in parserOnBody, after which its timer also stops refreshing — so the gap is that Node reads (and refreshes on) up to ~16KB before pausing, while Bun pauses immediately with zero refreshes.

    How to fix

    Same class as the other open comments — refresh the JS timer on every activity path. Options, cheapest first:

    1. Add this.socket?._unrefTimer?.() in onIncomingMessageResumeNodeHTTPResponse (after handle.resume() returns data) and after handle.drainRequestBody() in _read, so the initial buffered chunk counts as activity.
    2. In _onTimeout, also check the paused-body buffer size (e.g. expose buffered_request_body_data_during_pause.len() alongside bufferedAmount) and treat a non-empty inbound buffer as "write in progress".
    3. Cleanest: mirror Node and refresh from the native data-received path regardless of pause state — have the paused-buffer shim bump a JS-visible activity signal.

    Filed as a nit: same bug class the author is already addressing per the other three open comments, narrow trigger (handler delays body consumption + short server.setTimeout with no listener), and the regression is Windows-only in practice.

…m, refresh on buffered-body drain

_onTimeout now compares bufferedAmount against the last-observed value and
only suppresses 'timeout' when they differ (progress), so a fully-stalled
write still emits 'timeout' instead of looping the refresh forever.

Arm the socket timer only on the first request on a connection (isSocketNew),
matching Node.js's connectionListenerInternal, so a user 'connection'
listener or earlier-request res.setTimeout is not re-clobbered on subsequent
keep-alive requests; later requests only refresh.

Refresh the socket timer when the initial paused-body buffer is drained in
onIncomingMessageResumeNodeHTTPResponse and _read's drainRequestBody, so
body bytes that arrived while the native handle was paused still count as
activity once consumption starts.
@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the three additional findings in d066a09:

  • _onTimeout progress check: #lastBufferedAmount is tracked and the timeout is only suppressed when bufferedAmount has changed since the last fire, so a fully-stalled write still emits 'timeout'.
  • isSocketNew gate: the arm is already above emit('connection') as of 4398e28; now also gated on isSocketNew so subsequent keep-alive requests only _unrefTimer() and a user 'connection'-listener override sticks.
  • Paused-body drain refresh: onIncomingMessageResumeNodeHTTPResponse and _read's drainRequestBody path now refresh the timer when the initial buffered chunk is delivered.

@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.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/js/node/_http_server.ts:1189-1195 — This guard now suppresses 'timeout' whenever bufferedAmount > 0, but unlike Node's net.Socket._onTimeout it doesn't check whether the buffer is draining — Node compares this[kLastWriteQueueSize] against the current handle.writeQueueSize and only suppresses when they differ. A client that sends a request head then stops reading (slow-read / half-open TCP) leaves bufferedAmount stuck >0, so _onTimeout self-refreshes via _unrefTimer() every N ms and never emits — server.setTimeout(N) is defeated for exactly the stalled-connection case it exists to reap. Track the previous bufferedAmount on the socket and only suppress when it has changed, or drop this guard entirely (the write-path _unrefTimer() calls added in 36e1b22 already keep an actively-writing response alive).

    Extended reasoning...

    What the bug is

    Commit 36e1b22 changed this guard from response.writableLength > 0 (always undefined > 0 → false, dead code) to response.bufferedAmount > 0 (a real getter on NodeHTTPResponse, now live) and simultaneously wired _onTimeout up to a real timer via #timeoutTimer. The guard suppresses the timeout and re-arms the timer whenever any bytes are buffered on the socket, with no check for whether the write queue is actually making progress.

    Node.js's equivalent, net.Socket.prototype._onTimeout (lib/net.js), does this instead:

    const lastWriteQueueSize = this[kLastWriteQueueSize];
    if (lastWriteQueueSize > 0 && handle) {
      const { writeQueueSize } = handle;
      if (lastWriteQueueSize !== writeQueueSize) {
        this[kLastWriteQueueSize] = writeQueueSize;
        this._unrefTimer();
        return;
      }
    }
    this.emit('timeout');

    It only suppresses when the queue size has changed since the last check — i.e., the write is draining. If the queue is nonzero but unchanged (client stopped ACKing, TCP zero-window, network partition without RST, slow-read attacker), Node falls through to emit('timeout') so the default destroy can reap the socket. There is no equivalent last-buffered-amount tracking anywhere in src/js/.

    Step-by-step proof

    const srv = http.createServer((req, res) => res.end(Buffer.alloc(1 << 20)));
    srv.setTimeout(1000);   // no 'timeout' listener → default destroy
    1. Client connects, sends GET / HTTP/1.1\r\nHost: a\r\n\r\n, then never reads the response (or its route dies without a FIN/RST). The 1 MB write backpressures; response.bufferedAmount sits at some nonzero value that never decreases.
    2. onNodeHTTPRequest armed socket.setTimeout(1000) at line 616, so #timeoutTimer fires _onTimeout() at t≈1000ms.
    3. handle.response is the native NodeHTTPResponse (still attached — get_buffered_amount() returns 0 only when REQUEST_HAS_COMPLETED || SOCKET_CLOSED, and REQUEST_HAS_COMPLETED is set by uWS when the response is fully flushed, not when handle.end() is called). bufferedAmount > 0 → this._unrefTimer() → #timeoutTimer.refresh() → return.
    4. At t≈2000, 3000, … the same thing happens. 'timeout' never fires, onNodeHTTPServerSocketTimeout never runs, the socket stays open until the OS TCP retransmit timer gives up (~15 min on Linux with default tcp_retries2).

    In Node.js the same program emits 'timeout' at t≈2000ms (the second _onTimeout sees writeQueueSize unchanged from the first) and destroys the socket.

    Why nothing else prevents it

    Every other _unrefTimer() call site this PR added — ServerResponse.write/_send (lines 2214/2379), allowWritesToContinue (line 2529), _write (line 1361), #onData/#onDrain (lines 1089/1100), onDataIncomingMessage, endSocketOnFinishIfNeeded — requires activity to fire. A truly stalled backpressured write generates none of those events, so _onTimeout is the only thing running, and it self-perpetuates. The native uWS idle timeout is not a fallback: this PR removed setServerIdleTimeout and the server passes idleTimeout: 0 to Bun.serve.

    Why this is technically a regression

    Before this PR, server.setTimeout(N) armed the native uWS per-response idle timer (resp.timeout(config.idle_timeout) at mod.rs:1160), which resets on socket-level I/O. A stalled write produces no socket-level I/O, so uWS's HttpContext::onTimeout fired and unconditionally close()d the socket after ~N seconds. After this PR that path is gone and only the JS guard runs, which loops. (The pre-PR behavior was itself broken in the way this PR fixes — no listener veto, no idle-keepalive event — so this is a narrow regression inside a large net improvement.)

    Impact

    server.setTimeout(N) is defeated for the slow-read / half-open-connection case it exists to reap. It's a resource-exhaustion vector but requires a backpressure-sized response and a non-reading client; nothing crashes or corrupts. Filed as a nit given the narrow trigger and trivial fix.

    Fix

    Match Node — track the previous buffered amount on the socket and only suppress when it changed:

    _onTimeout() {
      const response = this[kHandle]?.response;
      const buffered = response ? response.bufferedAmount : 0;
      if (buffered > 0 && buffered !== this.#lastBufferedAmount) {
        this.#lastBufferedAmount = buffered;
        this._unrefTimer();
        return;
      }
      this.#lastBufferedAmount = 0;
      this.emit('timeout');
    }

    Alternatively, just drop the guard: the write-path _unrefTimer() calls added in 36e1b22 (ServerResponse.write/_send/allowWritesToContinue) already keep an actively-writing response from timing out, which is the case this guard was meant to cover.

Comment thread src/js/internal/http.ts Outdated
IncomingMessage.setTimeout delegates to socket.setTimeout since 4398e28,
so setRequestTimeout has no JS consumer. Remove it from internal/http.ts
and the jsHTTPSetTimeout host function / putDirect / extern decls from
NodeHTTP.cpp, along with the Rust-side Request__setTimeout and
NodeHTTPResponse__setTimeout exports it called.
@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

The _onTimeout presence-vs-progress finding was already addressed in d066a09 (#lastBufferedAmount tracking; suppress only when the buffered amount has changed since the last fire). 2f5af30 drops the now-dead setRequestTimeout binding chain.

@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

CI on 3d84d87 is red on two lanes unrelated to this diff:

  • test/bake/dev-and-prod.test.ts (Windows x64-baseline, HMR socket disconnect flake, auto-retried)
  • test/js/web/fetch/blob.test.ts (linux x64-baseline, EADDRINUSE port 3000 in use)

test/js/node/http/node-http-server-setTimeout.test.ts and the two updated test/js/bun/test/parallel/test-http-should-emit-timeout-event* tests pass on every lane. The diff is green; needs a maintainer to review/merge.

Comment thread src/js/node/_http_server.ts
Comment thread src/runtime/webcore/Request.rs
NodeHTTPServerSocket.setTimeout now validates msecs with getTimerDuration
(like net.Socket.setTimeout), so invalid input throws ERR_INVALID_ARG_TYPE
or ERR_OUT_OF_RANGE synchronously instead of arming a ~1ms timer and
silently destroying the socket on the next tick. Also early-return on a
destroyed socket, matching the sibling in net.ts.

Request::set_timeout and its c_uint import became dead once ffi_set_timeout
was removed in 2f5af30; drop them.

@coderabbitai coderabbitai 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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/js/node/_http_server.ts (1)

1345-1358: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate timeout callbacks by presence, not truthiness. setTimeout() should treat null, false, and 0 as invalid callbacks here; gate on callback !== undefined, validate once with $isCallable/$ERR_INVALID_ARG_TYPE, and use the same presence check for both once() and removeListener().

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js/node/_http_server.ts` around lines 1345 - 1358, The setTimeout method
in _http_server.ts is checking callback truthiness instead of presence, which
can let invalid values like null, false, or 0 slip through. Update the callback
handling in setTimeout() to gate on callback !== undefined, validate the
provided callback once with $isCallable and $ERR_INVALID_ARG_TYPE before using
it, and apply the same presence-based check consistently for both
once("timeout", callback) and removeListener("timeout", callback).

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/js/node/_http_server.ts`:
- Around line 1345-1358: The setTimeout method in _http_server.ts is checking
callback truthiness instead of presence, which can let invalid values like null,
false, or 0 slip through. Update the callback handling in setTimeout() to gate
on callback !== undefined, validate the provided callback once with $isCallable
and $ERR_INVALID_ARG_TYPE before using it, and apply the same presence-based
check consistently for both once("timeout", callback) and
removeListener("timeout", callback).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d84621a7-4978-4fca-8d2d-3f4f8cad61ea

📥 Commits

Reviewing files that changed from the base of the PR and between 3d84d87 and 06dcdc1.

📒 Files selected for processing (2)
  • src/js/node/_http_server.ts
  • src/runtime/webcore/Request.rs
💤 Files with no reviewable changes (1)
  • src/runtime/webcore/Request.rs

Comment thread src/js/node/_http_server.ts
@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

CI on b7f981c is red on two lanes unrelated to this diff:

  • test/cli/install/bun-install.test.ts (Windows x64, EBADF fstat in tarball-path test; same flake seen on the earlier builds)
  • test/regression/issue/26030.test.ts (alpine x64-baseline, MySQL docker coordinator startup failure)

test/js/node/http/node-http-server-setTimeout.test.ts and the two updated test-http-should-emit-timeout-event* parallel tests pass on every lane. All review threads are resolved; ready for a maintainer.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-08, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
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.

1 participant