Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/bun-uws/src/App.h
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,10 @@ struct TemplatedApp {
void setOnSocketClosed(HttpContextData<SSL>::OnSocketClosedCallback onClose) {
httpContext->getSocketContextData()->onSocketClosed = onClose;
}
void setOnSocketOpen(HttpContextData<SSL>::OnSocketOpenCallback onOpen, void *userData) {
httpContext->getSocketContextData()->onSocketOpen = onOpen;
httpContext->getSocketContextData()->onSocketOpenUserData = userData;
}
void setOnSocketDrain(HttpContextData<SSL>::OnSocketDrainCallback onDrain) {
httpContext->getSocketContextData()->onSocketDrain = onDrain;
}
Expand Down
8 changes: 8 additions & 0 deletions packages/bun-uws/src/HttpContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,10 @@ struct HttpContext {
for (auto &f : httpContextData->filterHandlers) {
f((HttpResponse<SSL> *) s, 1);
}

if (httpContextData->onSocketOpen) {
httpContextData->onSocketOpen(httpContextData->onSocketOpenUserData, SSL, s);
}
}
}

Expand All @@ -194,6 +198,10 @@ struct HttpContext {
for (auto &f : httpContextData->filterHandlers) {
f((HttpResponse<SSL> *) s, 1);
}

if (httpContextData->onSocketOpen) {
httpContextData->onSocketOpen(httpContextData->onSocketOpenUserData, SSL, s);
}
}

return s;
Expand Down
3 changes: 3 additions & 0 deletions packages/bun-uws/src/HttpContextData.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ struct alignas(16) HttpContextData {
using OnSocketUpgradedCallback = void (*)(void* userData, int is_ssl, struct us_socket_t *rawSocket);
using OnClientErrorCallback = MoveOnlyFunction<void(int is_ssl, struct us_socket_t *rawSocket, uWS::HttpParserError errorCode, char *rawPacket, int rawPacketLength)>;
using OnSocketClosedCallback = void (*)(void* userData, int is_ssl, struct us_socket_t *rawSocket);
using OnSocketOpenCallback = void (*)(void* ctxUserData, int is_ssl, struct us_socket_t *rawSocket);

MoveOnlyFunction<void(const char *hostname)> missingServerNameHandler;

Expand All @@ -68,6 +69,8 @@ struct alignas(16) HttpContextData {
OnSocketDataCallback onSocketData = nullptr;
OnSocketUpgradedCallback onSocketUpgraded = nullptr;
OnClientErrorCallback onClientError = nullptr;
OnSocketOpenCallback onSocketOpen = nullptr;
void *onSocketOpenUserData = nullptr;

uint64_t maxHeaderSize = 0; // 0 means no limit

Expand Down
1 change: 1 addition & 0 deletions src/js/internal/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const {
useStrictMethodValidation: boolean,
maxHeaderSize: number,
onClientError: (ssl: boolean, socket: any, errorCode: number, rawPacket: ArrayBuffer) => undefined,
onSocketOpen?: (ssl: boolean, socket: any) => undefined,
) => void;
getCompleteWebRequestOrResponseBodyValueAsArrayBuffer: (arg: any) => ArrayBuffer | undefined;
drainMicrotasks: () => void;
Expand Down
160 changes: 136 additions & 24 deletions src/js/node/_http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
kRealListen,
tlsSymbol,
optionsSymbol,
kDeferredTimeouts,
kDeprecatedReplySymbol,
Comment on lines 27 to 30

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.

🟡 This PR removes the only consumers of kDeferredTimeouts (the import here and the deferred-replay loop in kRealListen/setTimeout), but the symbol is still declared at src/js/internal/http.ts:92 and exported at :547 with no remaining importers. The setServerIdleTimeout re-export in internal/http.ts (:14/:33/:590) is similarly orphaned now that _http_server.ts no longer imports it. Per the root CLAUDE.md ("Delete dead code in the same PR that makes it dead"), both should be removed in this PR.

Extended reasoning...

What

Before this PR, src/js/node/_http_server.ts imported kDeferredTimeouts from internal/http and used it in two places: Server.prototype[kRealListen] replayed any {msecs, callback} entries queued in this[kDeferredTimeouts] once the native server existed, and Server.prototype.setTimeout pushed onto that array when called before listen(). This PR rewrites setTimeout to simply store this.timeout = msecs (which is read per-socket at accept time), so the deferred queue is no longer needed and both the import and the replay loop are deleted.

That deletion was the only consumer of kDeferredTimeouts. After this PR, a repo-wide grep shows exactly two remaining hits, both in src/js/internal/http.ts:

src/js/internal/http.ts:92:const kDeferredTimeouts = Symbol("deferredTimeouts");
src/js/internal/http.ts:547:  kDeferredTimeouts,

No file imports it. The symbol is now an unused module-level const plus an unused named export.

The same applies to setServerIdleTimeout: this PR removes its only JS-side import (_http_server.ts previously called it from Server.prototype.setTimeout). After the PR, the only remaining JS references are the destructure from the native binding at internal/http.ts:14, the type annotation at :33, and the re-export at :590 — none of which has a downstream importer. (The native binding entry in NodeHTTP.cpp and the underlying Server__setIdleTimeout may have reasons to stay for Bun.serve, but the internal/http.ts re-export is dead.)

Step-by-step proof

  1. Pre-PR: rg kDeferredTimeouts → 4 hits: declaration + export in internal/http.ts, import + two uses in _http_server.ts.
  2. This diff removes the _http_server.ts import (line 30 of the old file) and both use sites (the if (this[kDeferredTimeouts]) replay block in kRealListen and the (this[kDeferredTimeouts] ??= []).push(...) branch in setTimeout).
  3. Post-PR: rg kDeferredTimeouts → only internal/http.ts:92 and :547. Nothing reads either.
  4. Likewise rg setServerIdleTimeout post-PR → internal/http.ts:14/33/590 and the native NodeHTTP.cpp binding registration; no JS importer of the internal/http export remains.

Why existing code doesn't prevent it

internal/http.ts is bundled into the binary as an internal module, so an unused export is not flagged by any tooling — it just ships dead. Public/exported items escape dead-code lints, which is exactly why the project guideline at CLAUDE.md:237 calls this out:

Delete dead code in the same PR that makes it dead (required scope — name the deletions in the description): … Public items escape dead-code lints — grep for callers manually.

Impact

None at runtime — an unused Symbol() const and an unused export entry have negligible cost. This is purely a code-cleanliness / project-convention issue, hence nit severity. The cost of not cleaning it up is that the next reader of internal/http.ts has to grep to discover the symbol is dead, and a future PR may accidentally resurrect the old (broken) deferral pattern by finding the symbol still exported.

Fix

In src/js/internal/http.ts:

  • Delete line 92 (const kDeferredTimeouts = Symbol("deferredTimeouts");).
  • Delete the kDeferredTimeouts, entry from the export list at line 547.
  • Delete setServerIdleTimeout from the destructure at line 14, the type at line 33, and the export list at line 590.

No test changes needed.

headerStateSymbol,
NodeHTTPHeaderState,
Expand All @@ -51,7 +50,6 @@
eofInProgress,
runSymbol,
drainMicrotasks,
setServerIdleTimeout,
setServerCustomOptions,
getMaxHTTPHeaderSize,
fakeSocketSymbol,
Expand Down Expand Up @@ -88,6 +86,7 @@
const ObjectKeys = Object.keys;
const MathMin = Math.min;
const MathFloor = Math.floor;
const DateNow = Date.now;

let cluster;

Expand Down Expand Up @@ -241,6 +240,7 @@
this._unref = false;
this.maxRequestsPerSocket = 0;
this.maxHeadersCount = null;
this.timeout = 0;
this[kInternalSocketData] = undefined;
this[kTrackedConnections] = new Set();
this[tlsSymbol] = null;
Expand Down Expand Up @@ -324,13 +324,12 @@
// Like Node.js's setupConnectionsTracking: each 'listening' event replaces
// the connections-checking interval timer (used by the headers/request
// timeout machinery) and destroys the previous one.
function noopConnectionsCheck() {}
function setupConnectionsTracking(this: any) {
if (this[kConnectionsCheckingInterval]) {
clearInterval(this[kConnectionsCheckingInterval]);
}
const delay = this.connectionsCheckingInterval || 30_000;
this[kConnectionsCheckingInterval] = setInterval(noopConnectionsCheck, delay);
this[kConnectionsCheckingInterval] = setInterval(checkConnections.bind(this), delay);
this[kConnectionsCheckingInterval].unref();
}

Expand Down Expand Up @@ -593,6 +592,17 @@
if (!socket) {
socket = new NodeHTTPServerSocket(server, socketHandle, !!tls);
}
// A parsed request head is activity: headers just completed, the
// request-start anchor advances for a keep-alive reuse, and the
// inactivity timer switches back from keepAliveTimeout to
// server.timeout (like Node.js's parserOnIncoming).
socket[kHeadersCompleted] = true;
if (socket[kRequestStart] === 0) socket[kRequestStart] = DateNow();
if (socket.timeout !== server.timeout) {

Check failure on line 601 in src/js/node/_http_server.ts

View workflow job for this annotation

GitHub Actions / Lint JavaScript

bun(no-duplicate-conditional-property-access)

`server.timeout` is read in the `if` condition and again in the body. Read it into a local first (e.g. `const { timeout } = server`) so the property is only accessed once.
socket.setTimeout(server.timeout || 0);
} else {
socket._unrefTimer();
}
Comment on lines +599 to +605

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.

🔴 The per-socket inactivity timer is armed once at the request head (line 604) but never reset as request body chunks arrive, so server.setTimeout(ms) behaves as a wall-clock timer rather than an inactivity timer during uploads. With server.setTimeout(5000), a client uploading a 30-second body in continuous chunks will have 'timeout' fire after 5s and the socket destroyed mid-upload, even though bytes are actively flowing — Node.js resets the timer on every incoming read. Fix: call socket._unrefTimer() in onDataIncomingMessage (or wherever body chunks are pushed to the IncomingMessage).

Extended reasoning...

What the bug is

server.setTimeout(msecs) is documented (and behaves in Node.js) as an inactivity timer: it fires only when no bytes have moved on the socket for msecs milliseconds. In Node.js, every incoming chunk goes through onStreamRead, which calls stream[kUpdateTimer]() → socket._unrefTimer(), refreshing the timer. So an actively-uploading client never trips server.timeout no matter how long the total upload takes.

In this PR the timer is armed exactly once per request, at the head:

// _http_server.ts onNodeHTTPRequest, lines 599-605
socket[kHeadersCompleted] = true;
if (socket[kRequestStart] === 0) socket[kRequestStart] = DateNow();
if (socket.timeout !== server.timeout) {
  socket.setTimeout(server.timeout || 0);
} else {
  socket._unrefTimer();
}

After that, request body chunks flow through handle.ondata → onDataIncomingMessage (_http_incoming.ts:370-386), which only does this.push(chunk) and never touches socket._unrefTimer(). There are no _unrefTimer calls anywhere in _http_incoming.ts, and for normal (non-CONNECT/upgrade) requests socket[kEnableStreaming](false) clears the socket's own handle.ondata, so the body bypasses NodeHTTPServerSocket.#onData entirely.

Why existing code doesn't prevent it

_onTimeout()'s only suppression check is for outgoing backpressure:

if (response && response.writableLength > 0) {
  this._unrefTimer();
  return;
}

It does not check whether incoming reads are in progress, so an upload with an empty response buffer falls straight through to this.emit('timeout') → onNodeHTTPServerSocketTimeout, which destroys the socket if no listener swallows it (or hands it to the user's server.setTimeout callback, which conventionally also destroys it).

Bun's own net.ts Socket does call _unrefTimer() on every data chunk, matching Node — it's only the NodeHTTPServerSocket body path that misses it.

Step-by-step proof

  1. const server = http.createServer((req, res) => { req.pipe(res); }); server.setTimeout(5000, s => s.destroy()); server.listen(0);
  2. Client connects → onServerSocketOpen → socket.setTimeout(5000) arms kTimeoutHandle = setTimeout(socketOnTimeoutFire, 5000, socket).
  3. Client sends POST / HTTP/1.1\r\nHost: a\r\nContent-Length: 30000\r\n\r\n → onNodeHTTPRequest runs, calls socket._unrefTimer() once (re-arms 5000ms).
  4. Client sends 1KB of body every 1s for 30s. Each chunk reaches onDataIncomingMessage → this.push(chunk). Nothing calls socket._unrefTimer().
  5. At t≈5s after the head, socketOnTimeoutFire(socket) → socket._onTimeout() → response.writableLength is likely 0 (small chunks flush immediately) → this.emit('timeout') → onNodeHTTPServerSocketTimeout → server's 'timeout' listener destroys the socket.
  6. The upload is aborted at byte ~5000 of 30000 even though bytes were arriving every second. Node.js would never fire 'timeout' here because each chunk would have reset the 5s timer.

Impact

Any node:http server that calls server.setTimeout(N) (or sets server.timeout = N) and accepts uploads longer than N ms will spuriously destroy active uploads. This is opt-in (default server.timeout is 0), but it's the very feature this PR exists to implement, and server.setTimeout + file uploads is a common combination. It is not a regression (socket.setTimeout was a no-op before), but it makes the newly-implemented knob incorrect for the upload case.

Fix

Call this.socket?._unrefTimer() inside onDataIncomingMessage whenever a chunk arrives (mirroring Node's onStreamRead → kUpdateTimer), e.g.:

function onDataIncomingMessage(chunk, isLast, aborted) {
  if (aborted === NodeHTTPResponseAbortEvent.abort) { this.destroy(); return; }
  this.socket?._unrefTimer();
  if (chunk && !this._dumped) this.push(chunk);
  if (isLast) emitEOFIncomingMessage(this);
}

(or equivalently in the NodeHTTPResponse data path). Outgoing writes should similarly reset the timer, but that's a separate concern from this finding.


const http_req = new RequestClass(kHandle, url, method, headersObject, headersArray, handle, hasBody, socket);
if (isAncientHTTP) {
Expand Down Expand Up @@ -646,7 +656,7 @@
if (!requestShouldKeepAlive(http_req)) {
http_res[kMustCloseConnection] = true;
}
http_res.once("finish", endSocketOnFinishIfNeeded.bind(undefined, socket, http_res));
http_res.once("finish", resOnFinish.bind(undefined, server, socket, http_res));

if (hasObserver("http")) {
startPerf(http_res, kServerResponseStatistics, {
Expand All @@ -665,6 +675,14 @@

setIsNextIncomingMessageHTTPS(prevIsNextIncomingMessageHTTPS);
handle.onabort = onServerRequestEvent.bind(socket);
// Like Node.js, the requestTimeout clock runs until the incoming
// request (not the response) is complete: a body-less request is
// complete at the head, and a body stops the clock at its 'end'.
if (!hasBody) {
socket[kRequestStart] = 0;
} else {
http_req.once("end", clearIncomingRequestTimer.bind(undefined, socket));
}
// start buffering data if any, the user will need to resume() or .on("data") to read it
if (hasBody) {
handle.pause();
Expand Down Expand Up @@ -692,7 +710,8 @@
}
}

if (isSocketNew && !reachedRequestsLimit) {
if (!socket[kConnectionEmitted] && !reachedRequestsLimit) {
socket[kConnectionEmitted] = true;
server.emit("connection", socket);
}

Expand Down Expand Up @@ -892,6 +911,7 @@
true,
typeof this.maxHeaderSize !== "undefined" ? this.maxHeaderSize : getMaxHTTPHeaderSize(),
onServerClientError.bind(this),
onServerSocketOpen.bind(this),
);

if (this?._unref) {
Expand All @@ -902,25 +922,13 @@
this.once("listening", onListen);
}

if (this[kDeferredTimeouts]) {
for (const { msecs, callback } of this[kDeferredTimeouts]) {
this.setTimeout(msecs, callback);
}
delete this[kDeferredTimeouts];
}

setTimeout(emitListeningNextTick, 1, this, this[serverSymbol]?.hostname, this[serverSymbol]?.port);
}
};

Server.prototype.setTimeout = function (msecs, callback) {
const server = this[serverSymbol];
if (server) {
setServerIdleTimeout(server, Math.ceil(msecs / 1000));
if (typeof callback === "function") this.once("timeout", callback);
} else {
(this[kDeferredTimeouts] ??= []).push({ msecs, callback });
}
this.timeout = msecs;
if ($isCallable(callback)) this.on("timeout", callback);
return this;
};

Expand Down Expand Up @@ -953,6 +961,67 @@
HTTP_PARSER_ERROR_INVALID_METHOD = 9,
HTTP_PARSER_ERROR_INVALID_HEADER_TOKEN = 10,
}
// Fired by the native side for every accepted connection (after the TLS
// handshake for https), before any bytes are parsed. This is what lets the
// headers/request/inactivity timeouts cover sockets that never send a full
// request, and it makes 'connection' fire on accept like Node.js.
function onServerSocketOpen(this: Server, ssl: boolean, handle: unknown) {
const self = this;
// The native side returns the existing wrapper for a handle it has seen
// before; a second Duplex would strand the first in kTrackedConnections.
const existingDuplex = (handle as any).duplex;
const socket = existingDuplex ?? new NodeHTTPServerSocket(self, handle, ssl);
Comment on lines +968 to +973

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.

🟡 The comment on Server.prototype.getConnections ("Connections are tracked from the first parsed request on each socket (the native server does not surface raw accepts to JS yet)") is now stale: this PR adds onServerSocketOpen, which fires on accept and constructs a NodeHTTPServerSocket whose constructor adds it to kTrackedConnections. Both halves of the comment are now false — please update or drop it.

Extended reasoning...

What the issue is

Server.prototype.getConnections (src/js/node/_http_server.ts:361-362) carries this comment:

// Connections are tracked from the first parsed request on each socket
// (the native server does not surface raw accepts to JS yet).

This PR directly invalidates both factual claims in that sentence, but leaves the comment untouched.

The code path that invalidates it

  1. This PR adds an onSocketOpen hook to uWS (HttpContext.h onOpen/onHandshake), plumbed through uws_app_set_on_socket_open → App::on_socket_open → Server__setOnNodeSocketOpen → the new sixth argument of setServerCustomOptions.
  2. Server.prototype[kRealListen] now passes onServerSocketOpen.bind(this) as that sixth argument.
  3. onServerSocketOpen runs on every accepted connection (after the TLS handshake for https), before any bytes are parsed, and does new NodeHTTPServerSocket(self, handle, ssl).
  4. The NodeHTTPServerSocket constructor calls server[kTrackedConnections]?.add(this).

So after this PR: connections are tracked from accept (not from the first parsed request), and the native server does surface raw accepts to JS. Both halves of the comment are now factually wrong.

Step-by-step proof

  • Client connects but sends nothing.
  • uWS onOpen fires → httpContextData->onSocketOpen(...) → Rust thunk → on_node_socket_open_callback → JS onServerSocketOpen.
  • onServerSocketOpen constructs NodeHTTPServerSocket, whose constructor runs server[kTrackedConnections]?.add(this).
  • server.getConnections(cb) now reports 1 for this socket even though no request head has been parsed — directly contradicting the comment's first clause.
  • The very existence of onServerSocketOpen being called from native on accept contradicts the parenthetical second clause.

Why nothing prevents it

The PR did not touch getConnections itself, so the comment was simply missed. There is no compile-time or test-time guard that ties comments to behavior.

Impact

Documentation accuracy only — no runtime effect. But it is a specific factual claim (not a vague description) that this PR's central change directly contradicts, and per repo convention comments invalidated by a PR should be updated in the same PR. A future reader debugging getConnections semantics would be actively misled about when tracking begins.

How to fix

Update the comment to reflect the new behavior, e.g.:

// Connections are tracked from accept (onServerSocketOpen adds the
// NodeHTTPServerSocket wrapper to kTrackedConnections before any bytes
// are parsed).

Or simply drop the comment if it no longer adds value.

// Anchor the headers/request timeout clock at accept. Node.js anchors at
// the first parsed byte, which we cannot observe; accept is never later.
socket[kRequestStart] = DateNow();
socket[kHeadersCompleted] = false;
const serverTimeout = self.timeout;
if (serverTimeout) socket.setTimeout(serverTimeout);
if (!socket[kConnectionEmitted]) {
socket[kConnectionEmitted] = true;
self.emit("connection", socket);
}
}

function clearIncomingRequestTimer(socket) {
socket[kRequestStart] = 0;
}

const requestTimeoutResponse = Buffer.from("HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n", "ascii");
// Mirrors Node.js's default 'clientError' handling of ERR_HTTP_REQUEST_TIMEOUT:
// user listeners take over; otherwise answer 408 (only if nothing was
// written yet) and destroy the socket.
function onRequestTimeout(server: Server, socket) {
socket[kRequestStart] = 0;
const err = $ERR_HTTP_REQUEST_TIMEOUT("Request timeout");
if (!server.emit("clientError", err, socket)) {
if (socket.writable && (socket.bytesWritten | 0) === 0) {
socket.write(requestTimeoutResponse);
}
socket.destroy(err);
}
}

// Node.js's checkConnections sweep: every connectionsCheckingInterval ms,
// close connections whose request head (headersTimeout) or entire request
// (requestTimeout) has not arrived in time.
function checkConnections(this: Server) {
const headersTimeout = this.headersTimeout;
const requestTimeout = this.requestTimeout;
if (headersTimeout === 0 && requestTimeout === 0) return;
const now = DateNow();
for (const socket of this[kTrackedConnections]) {
const start = socket[kRequestStart];
if (!start) continue;
const elapsed = now - start;
if (headersTimeout > 0 && !socket[kHeadersCompleted] && elapsed > headersTimeout) {
onRequestTimeout(this, socket);
} else if (requestTimeout > 0 && elapsed > requestTimeout) {
onRequestTimeout(this, socket);
}
}
}

function onServerClientError(ssl: boolean, socket: unknown, errorCode: number, rawPacket: ArrayBuffer) {
const self = this as Server;
let err;
Expand Down Expand Up @@ -989,7 +1058,8 @@
// connections - the existing duplex already had its 'connection' event.
const existingDuplex = (socket as any).duplex;
const nodeSocket = existingDuplex ?? new NodeHTTPServerSocket(self, socket, ssl);
if (!existingDuplex) {
if (!nodeSocket[kConnectionEmitted]) {
nodeSocket[kConnectionEmitted] = true;
self.emit("connection", nodeSocket);
}
self.emit("clientError", err, nodeSocket);
Expand All @@ -1000,6 +1070,15 @@

const kBytesWritten = Symbol("kBytesWritten");
const kEnableStreaming = Symbol("kEnableStreaming");
const kTimeoutHandle = Symbol("kTimeoutHandle");
const kRequestStart = Symbol("kRequestStart");
const kHeadersCompleted = Symbol("kHeadersCompleted");
const kConnectionEmitted = Symbol("kConnectionEmitted");

function socketOnTimeoutFire(socket) {
socket[kTimeoutHandle] = null;
socket._onTimeout();
}
function onServerSocketError(this: any, _err) {
// Default 'error' listener so socket-level errors (e.g. res.destroy(err)
// forwarding the error to the socket) do not crash the process as
Expand All @@ -1021,6 +1100,10 @@
server: Server;
_httpMessage;
_secureEstablished = false;
[kTimeoutHandle] = null;
[kRequestStart] = 0;
[kHeadersCompleted] = false;
[kConnectionEmitted] = false;
#pendingCallback = null;
constructor(server: Server, handle, encrypted) {
super();
Expand Down Expand Up @@ -1098,6 +1181,11 @@
}
#onClose() {
this[kHandle] = null;
const timeoutHandle = this[kTimeoutHandle];
if (timeoutHandle !== null) {
clearTimeout(timeoutHandle);
this[kTimeoutHandle] = null;
}
this.server?.[kTrackedConnections]?.delete(this);

// Node.js's `socketOnClose` → `abortIncoming()` only destroys requests
Expand Down Expand Up @@ -1152,12 +1240,20 @@
// If there is a response, and it has pending data,
// we suppress the timeout because a write is in progress.
if (response && response.writableLength > 0) {
this._unrefTimer();
return;
}
this.emit("timeout");
}
_unrefTimer() {
// for compatibility
const msecs = this.timeout;
const handle = this[kTimeoutHandle];
if (handle !== null) clearTimeout(handle);
if (msecs > 0) {
this[kTimeoutHandle] = setTimeout(socketOnTimeoutFire, msecs, this).unref();
} else {
this[kTimeoutHandle] = null;
}
}

address() {
Expand Down Expand Up @@ -1293,7 +1389,10 @@
return this;
}

setTimeout(_timeout, _callback) {
setTimeout(msecs, callback) {
this.timeout = msecs | 0;
if ($isCallable(callback)) this.once("timeout", callback);
this._unrefTimer();
Comment on lines +1392 to +1395

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.

🟡 nit: msecs | 0 coerces to int32, so socket.setTimeout(2_500_000_000) (or any msecs >= 2**31, including server.keepAliveTimeout/server.timeout flowing through here) wraps to a negative number and _unrefTimer's if (msecs > 0) check then leaves the timer disarmed. Node.js's net.Socket.setTimeout (and Bun's own net.ts) instead runs getTimerDuration(msecs, 'msecs'), which clamps to TIMEOUT_MAX = 2**31 - 1 with a TimeoutOverflowWarning. The trigger is a >24.8-day inactivity timeout so the practical impact is negligible, but for parity consider this.timeout = getTimerDuration(msecs, 'msecs') (or at least >>> 0).

Extended reasoning...

What the bug is. NodeHTTPServerSocket.prototype.setTimeout stores the timeout as this.timeout = msecs | 0. The bitwise-OR coerces its operand to a signed 32-bit integer, so any msecs value at or above 2**31 (2147483648) wraps to a negative number — e.g. 2_500_000_000 | 0 === -1794967296. _unrefTimer() then reads this.timeout, sees msecs > 0 is false, and takes the else branch that sets this[kTimeoutHandle] = null instead of arming a setTimeout. The net effect is that asking for a very large inactivity timeout silently disables the timer entirely.

The specific code path. There are three entry points that reach this coercion: (1) a user calling req.socket.setTimeout(N) or res.setTimeout(N) directly on the server socket; (2) server.timeout = N / server.setTimeout(N), which onServerSocketOpen and onNodeHTTPRequest forward to socket.setTimeout(server.timeout); and (3) server.keepAliveTimeout = N, which resOnFinish forwards to socket.setTimeout(keepAliveTimeout) after each response. storeHTTPOptions validates keepAliveTimeout and requestTimeout with validateInteger(..., 0), whose default upper bound is Number.MAX_SAFE_INTEGER, so values ≥ 2^31 pass validation and reach the | 0 truncation unchanged.

Why nothing prevents it. Node.js's net.Socket.prototype.setTimeout (and Bun's own implementation in src/js/node/net.ts:2273) calls getTimerDuration(msecs, 'msecs') from internal/timers, which (a) validates the value is a non-negative number and (b) clamps anything above TIMEOUT_MAX = 2**31 - 1 down to TIMEOUT_MAX while emitting a TimeoutOverflowWarning. The new NodeHTTPServerSocket is a separate Duplex subclass that does not go through net.Socket.prototype.setTimeout, and its | 0 shortcut does not replicate that clamping. Before this PR setTimeout on this class was a no-op, so there was no prior behavior to regress; this is a new divergence introduced alongside the new functionality.

Step-by-step proof.

  1. server.keepAliveTimeout = 3_000_000_000 (≈ 34.7 days). storeHTTPOptions calls validateInteger(3_000_000_000, 'keepAliveTimeout', 0) → passes (≤ MAX_SAFE_INTEGER).
  2. A request is served; on response 'finish', resOnFinish(server, socket, res) runs with keepAliveTimeout = 3_000_000_000, which is truthy, so it calls socket.setTimeout(3_000_000_000).
  3. setTimeout does this.timeout = 3_000_000_000 | 0 → -1294967296, then calls this._unrefTimer().
  4. _unrefTimer reads msecs = -1294967296, clears any existing handle, evaluates msecs > 0 → false, sets this[kTimeoutHandle] = null. No timer is armed.
  5. In Node.js the same call would store socket.timeout = 2147483647, emit a TimeoutOverflowWarning, and arm a ~24.8-day unref'd timer.

Impact. The behavioral divergence is real but the practical impact is minimal. 2^31 ms ≈ 24.8 days; no realistic socket inactivity, keep-alive, or server timeout approaches that (defaults are 0 / 5 s; generous deployments use minutes). The functional difference between "no timer" and "timer fires after 24.8 days" is essentially nil — the peer, OS keepalive, or process restart will close the connection long before either fires. Node itself treats this range as out-of-bounds and only warns. This is why the refuting verifier deems it non-actionable; we agree it does not block the PR, but it is still a correctness/parity gap worth noting.

Fix. Match net.Socket.setTimeout: import getTimerDuration from internal/timers and write this.timeout = msecs === 0 ? 0 : getTimerDuration(msecs, 'msecs') (Node treats 0 as "disable" without going through getTimerDuration, and server.timeout || 0 already passes 0 for the disabled case). A lighter alternative that avoids the warning machinery is this.timeout = Math.min(msecs >>> 0, 2 ** 31 - 1), but getTimerDuration is the established pattern in this codebase.

return this;
}

Expand Down Expand Up @@ -1653,9 +1752,22 @@
}
}

function endSocketOnFinishIfNeeded(socket, res) {
function resOnFinish(server, socket, res) {
if (res[kMustCloseConnection]) {
socket?.end();
return;
}
if (!socket || socket.destroyed) return;
// Keep-alive idle between requests: the headers/request timeouts only
// apply while a request is in flight; the keep-alive inactivity timer
// takes over until the next request head arrives (Node.js's resOnFinish).
socket[kRequestStart] = 0;
socket[kHeadersCompleted] = false;
const keepAliveTimeout = server.keepAliveTimeout;
if (keepAliveTimeout) {
socket.setTimeout(keepAliveTimeout);
} else {
socket._unrefTimer();
}
Comment on lines +1760 to 1771

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.

🔴 When two HTTP/1.1 requests are pipelined in one packet and request #1's handler calls res.end() synchronously, request #1's deferred 'finish' → resOnFinish runs after onNodeHTTPRequest has set up request #2's timeout state on the same socket, and unconditionally clobbers it: kRequestStart is zeroed (so request #2 escapes requestTimeout) and socket.setTimeout(keepAliveTimeout) overwrites server.timeout (so a slow async handler on request #2 gets the default 5s keep-alive timer and onNodeHTTPServerSocketTimeout may destroy the socket mid-request). Node.js's resOnFinish guards this via state.outgoing.length === 0; the equivalent here is if (socket._httpMessage) return; before the keep-alive bookkeeping (since res1.end() already detachSocket'd, a non-null _httpMessage means res2 is attached).

Extended reasoning...

What the bug is

resOnFinish (src/js/node/_http_server.ts:1755-1771) unconditionally resets per-socket timeout state to "keep-alive idle":

socket[kRequestStart] = 0;
socket[kHeadersCompleted] = false;
const keepAliveTimeout = server.keepAliveTimeout;
if (keepAliveTimeout) socket.setTimeout(keepAliveTimeout);
else socket._unrefTimer();

But res.end() emits 'finish' via process.nextTick, so for sync-pipelined requests this runs after the next request's onNodeHTTPRequest has already armed the same fields on the same socket, clobbering them.

Code path

uWS explicitly supports sync pipelining (HttpContext.h onData → consumePostPadded: "We still support sync pipelining"). With two requests in one TCP segment:

  1. onNodeHTTPRequest(req1) runs. The user handler calls res1.end() synchronously.
  2. res1.end() (ServerResponse.prototype.end): the native handle.end() clears HTTP_RESPONSE_PENDING; detachSocket(socket) clears socket._httpMessage = null; 'finish' is scheduled via process.nextTick.
  3. onNodeHTTPRequest(req1) returns with handle.finished true → no promise → the parser continues.
  4. onNodeHTTPRequest(req2) runs on the same socket: sets socket[kHeadersCompleted] = true, socket[kRequestStart] = DateNow(), calls socket.setTimeout(server.timeout || 0), and assignSocketInternal(http_res2, socket) sets socket._httpMessage = res2.
  5. onNodeHTTPRequest(req2) then calls drainMicrotasks() (line ~691), which drains the nextTick queue (Bun's Bun__drainMicrotasksFromJS → GlobalObject::drainMicrotasks() drains nextTickQueue).
  6. res1's 'finish' fires now → resOnFinish(server, socket, res1): res1[kMustCloseConnection] is false and socket is alive, so it writes socket[kRequestStart] = 0, socket[kHeadersCompleted] = false, and socket.setTimeout(server.keepAliveTimeout) (default 5000 ms).

Why nothing prevents it

Node.js's resOnFinish guards keep-alive arming with else if (state.outgoing.length === 0) — i.e. only when no further response is queued on the socket. This implementation has no such guard; it assumes 'finish' for response N always precedes the bookkeeping for request N+1, which sync pipelining violates.

Impact

For request #2 on the pipelined socket:

  • requestTimeout is bypassed. checkConnections does const start = socket[kRequestStart]; if (!start) continue; — with kRequestStart zeroed, request Fix calling #private() functions in classes #2 is never checked even if its body stalls forever.
  • keepAliveTimeout (default 5s) replaces server.timeout. If request Fix calling #private() functions in classes #2's handler is async and takes > 5s with no 'timeout' listener on req/res/server, socketOnTimeoutFire → _onTimeout → onNodeHTTPServerSocketTimeout calls this.destroy(), dropping a live in-flight request.

(Note: kHeadersCompleted=false does not spuriously trip headersTimeout, because kRequestStart=0 makes the sweep skip the socket entirely.)

The trigger — HTTP/1.1 pipelining with a synchronously-handled first request — is uncommon in browsers but reachable from raw TCP clients, proxies, and load-test tools, and the maxRequestsPerSocket code path's own comment acknowledges "already-pipelined requests, which still need to be dispatched".

Step-by-step proof

  1. Client sends in one packet: GET /a HTTP/1.1\r\nHost:x\r\n\r\nPOST /b HTTP/1.1\r\nHost:x\r\nContent-Length:50\r\n\r\n (no body bytes for /b yet).
  2. Handler for /a does res.end('ok') synchronously.
  3. Parser dispatches /b: onNodeHTTPRequest sets socket[kRequestStart]=now, socket[kHeadersCompleted]=true, socket.setTimeout(server.timeout||0), attaches res2, then drainMicrotasks().
  4. Inside drainMicrotasks(), res1 'finish' fires → resOnFinish zeroes kRequestStart, sets kHeadersCompleted=false, calls socket.setTimeout(5000).
  5. Client never sends /b's body. checkConnections sees start=0 → continue. requestTimeout never fires. Meanwhile, after 5s, socketOnTimeoutFire → onNodeHTTPServerSocketTimeout: req2.complete is false so it emits 'timeout' on req2; if nothing handles it, the socket is destroyed.

Fix

Skip the keep-alive idle bookkeeping when another response is already attached to the socket:

function resOnFinish(server, socket, res) {
  if (res[kMustCloseConnection]) { socket?.end(); return; }
  if (!socket || socket.destroyed) return;
  // The next pipelined request may already be attached; do not clobber its state.
  if (socket._httpMessage) return;
  socket[kRequestStart] = 0;
  socket[kHeadersCompleted] = false;
  ...
}

res1.end() calls detachSocket synchronously, so by the time res1's 'finish' runs, socket._httpMessage is non-null only if assignSocketInternal has attached res2 — exactly the case to guard.

}

Expand Down
1 change: 1 addition & 0 deletions src/jsc/bindings/ErrorCode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,5 +340,6 @@ const errors: ErrorCodeMapping = [
["ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY", Error],
["ERR_DIR_CONCURRENT_OPERATION", Error],
["ERR_INVALID_BUFFER_SIZE", RangeError],
["ERR_HTTP_REQUEST_TIMEOUT", Error],
];
export default errors;
Loading
Loading