Skip to content

node:http: wire server.timeout to the idle timeout - #33733

Closed
robobun wants to merge 2 commits into
mainfrom
farm/91cc47a4/http-server-timeout-property
Closed

robobun wants to merge 2 commits into
mainfrom
farm/91cc47a4/http-server-timeout-property

Conversation

@robobun

@robobun robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

What

http.Server.prototype.setTimeout mapped msecs onto the native idle timeout but never stored this.timeout, and there was no timeout accessor at all, so the two documented spellings of the same knob diverged:

  • server.timeout always read undefined (Node.js default: 0)
  • server.setTimeout(N) was enforced but did not update server.timeout
  • server.timeout = N was a plain expando the server never consulted: the app reads back N and gets no timeout

Repro

import http from 'node:http';
const s = http.createServer(() => {});
console.log(s.timeout);            // undefined (Node.js: 0)
s.setTimeout(1234);
console.log(s.timeout);            // undefined (Node.js: 1234)

And server.timeout = 500 after listen() leaves a stalled-body connection open indefinitely, where Node.js closes it after ~500 ms.

Fix

Add a timeout accessor on Server.prototype: the getter returns the stored msecs (initialized to 0), the setter stores the value and, when the native server is listening, routes through setServerIdleTimeout. setTimeout now writes through the accessor and, like Node.js, registers its callback with on('timeout', ...) rather than once. The deferred-timeouts array is no longer needed: the stored value is applied at listen time.

Verification

test/js/node/http/node-http-server-timeout.test.ts covers the property semantics (default 0, setTimeout writes it, assignment reads back) and that server.timeout = N after listen() actually closes a stalled connection. Both tests fail on the current release and pass with the fix. The existing test-http-should-emit-timeout-event-when-using-server-setTimeout still passes.

Server.prototype.setTimeout mapped msecs onto the native idle timeout but
never stored this.timeout, and there was no timeout accessor, so:

- server.timeout read undefined (Node.js default: 0)
- server.setTimeout(N) did not update server.timeout
- server.timeout = N was a plain expando the server never consulted

Add a timeout accessor on Server.prototype whose setter routes through
setServerIdleTimeout when the native server is listening, and store the
value so the getter matches Node.js. setTimeout now writes through the
accessor (and, like Node.js, registers the callback with on() rather than
once()). The deferred-timeouts array is no longer needed: the stored value
is applied at listen time.
@coderabbitai

coderabbitai Bot commented Jul 8, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: df577c90-692d-42c6-8c93-ee16424bf418

📥 Commits

Reviewing files that changed from the base of the PR and between e10d444 and 49fd885.

📒 Files selected for processing (3)
  • src/js/internal/http.ts
  • src/js/node/_http_server.ts
  • test/js/node/http/node-http-server-timeout.test.ts

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 4:04 AM PT - Jul 8th, 2026

❌ @autofix-ci[bot], your commit 49fd885 has 4 failures in Build #70357 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33733

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

bun-33733 --bun

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Fastify timeout settings not respected when using Bun runtime with NestJS and Fastify adapter #17287 - Fastify sets connectionTimeout and keepAliveTimeout via server.setTimeout() on the underlying node:http server, which previously had no effect because the timeout setter was a no-op

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

Fixes #17287

🤖 Generated with Claude Code

@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

This does not close #17287. Before this change server.setTimeout(N) already reached the native idle timeout (it was deferred before listen() and applied after), so Fastify's connectionTimeout path was not blocked by what is fixed here; and keepAliveTimeout is a separate property this PR does not touch.

What this PR changes is the server.timeout property itself: it now defaults to 0, reflects setTimeout(), and direct assignment (server.timeout = N) is honored instead of being an inert expando.

@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 socket/response setTimeout to the native inactivity timer #33732 - Both modify _http_server.ts to wire timeout to the native inactivity timer; node:http: wire server socket/response setTimeout to the native inactivity timer #33732 targets socket/response-level setTimeout, while this PR targets the server-level timeout property — overlapping code sections
  2. node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 - Also fixes Server.prototype.setTimeout to write through this.timeout, switches once to on("timeout"), and removes kDeferredTimeouts — the same core changes as this PR, within a larger compat omnibus

🤖 Generated with Claude Code

@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

On the duplicate-PR suggestions:

Comment thread test/js/node/http/node-http-server-timeout.test.ts
@robobun

robobun commented Jul 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI on build #70357 (final): the new node-http-server-timeout.test.ts passes on every lane, and all linux (glibc/musl, x64/aarch64, baseline, ASAN), alpine, debian, ubuntu, windows-2019-x64, windows-2019-x64-baseline, windows-11-aarch64 and darwin-26-aarch64 lanes are green.

Red is unrelated flake, each either reproduced on other open PRs today or outside node:http entirely:

  • test/js/third_party/express/res.send.test.ts (windows-2019-x64 first attempt, ECONNREFUSED on 3 supertest method variants): retry passed; also failing on build #70355.
  • test/js/bun/http/proxy-stress-concurrent.test.ts (darwin-14-x64, 1 of 1200 redirect requests failed): also failing on build #70340.
  • test/js/bun/terminal/terminal.test.ts (darwin-14-x64, Bun.spawn terminal attach timed out at 90 s): also failing on builds #70340 and #70343.
  • test/js/bun/webview/webview.test.ts (darwin-14-aarch64, WebView host process killed by SIGTRAP): native WKWebView host crash, not reachable from the JS-only change here.

None of these touch node:http. Ready for a maintainer.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

This landed on main through #32488. server.timeout is now a real property on the server (initialised to 0), server.setTimeout(ms, cb) just assigns it and registers the listener, and the value is applied to each connection when it is established, which is Node's model. Assigning server.timeout directly therefore takes effect the same way setTimeout() does.

Checked by running this PR's test/js/node/http/node-http-server-timeout.test.ts unmodified against a debug build of current main (05dd45e): 2 runs, 2 pass / 0 fail each time.

Nothing left for this PR to add, so closing it.

@robobun robobun closed this Aug 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