Skip to content

http_parser: multiply ConnectionsList#expired() timeouts in 64 bits #46411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

http_parser: multiply ConnectionsList#expired() timeouts in 64 bits #46411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions src/jsc/bindings/node/http/JSConnectionsListPrototype.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,10 @@ JSC_DEFINE_HOST_FUNCTION(jsConnectionsList_expired, (JSGlobalObject * globalObje
JSValue headersTimeoutValue = callFrame->argument(0);
JSValue requestTimeoutValue = callFrame->argument(1);

uint64_t headersTimeout = headersTimeoutValue.toUInt32(globalObject) * 1000000;
// Milliseconds to uv_hrtime() nanoseconds. A 32-bit product wraps above 4294 ms.
uint64_t headersTimeout = static_cast<uint64_t>(headersTimeoutValue.toUInt32(globalObject)) * 1000000;
RETURN_IF_EXCEPTION(scope, {});
uint64_t requestTimeout = requestTimeoutValue.toUInt32(globalObject) * 1000000;
uint64_t requestTimeout = static_cast<uint64_t>(requestTimeoutValue.toUInt32(globalObject)) * 1000000;
RETURN_IF_EXCEPTION(scope, {});

if (headersTimeout == 0 && requestTimeout == 0) {
Expand Down
34 changes: 34 additions & 0 deletions test/js/node/http/node-http-parser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,40 @@ describe("ConnectionsList", () => {
// to remove it.
expect(list.all()).toEqual([p1, p4, p3]);
});

// expired() takes milliseconds and compares uv_hrtime() nanoseconds. 4,227,858,433 ms is 48.9 days. In 32 bits
// its nanosecond count wraps to exactly 1 ms. The expected values are the results of Node v26.3.0.
const longTimeout = 4_227_858_433;
test.each([
[1, 0, ["incomplete"]],
[0, 1, ["incomplete", "complete"]],
[longTimeout, 0, []],
[0, longTimeout, []],
[longTimeout, longTimeout, []],
// expired() swaps the two timeouts when headersTimeout is above requestTimeout.
[longTimeout, 1, ["incomplete"]],
[1, longTimeout, ["incomplete"]],
])("expired(%d, %d) reports what Node reports", (headersTimeout, requestTimeout, expired) => {
const list = new ConnectionsList();
const incomplete = new HTTPParser();
incomplete.initialize(HTTPParser.REQUEST, {}, 0, 0, list);
const complete = new HTTPParser();
complete.initialize(HTTPParser.REQUEST, {}, 0, 0, list);
complete.execute(Buffer.from("POST / HTTP/1.1\r\nContent-Length: 1\r\n\r\n"));
// idle() lists a parser with no message in progress. duration() of such a parser stays 0.
expect({
headersCompleted: [incomplete.headersCompleted(), complete.headersCompleted()],
idle: list.idle(),
}).toEqual({ headersCompleted: [false, true], idle: [] });
// Both parsers are older than 1 ms.
while (incomplete.duration() < 2 || complete.duration() < 2) {}

@claude claude Bot Oct 21, 2026

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.

🟡 (optional) CI runners get a silent whole-file hang instead of a test failure if duration() ever stops advancing for one of the two parsers. The spin at node-http-parser.test.ts:480 has no deadline, and because it is synchronous the test runner's per-test timeout cannot fire. Any future regression in HTTPParser::duration() or in initialize() setting m_lastMessageStart (NodeHTTPParser.cpp:278) turns this test from red into a stuck job. Fix: bound the poll with a wall-clock deadline (for example a performance.now() limit of a few hundred ms) and fail with a message naming which parser's duration() stayed below 2, keeping the condition-polled shape.

Why this was flagged

The loop while (incomplete.duration() < 2 || complete.duration() < 2) {} at test/js/node/http/node-http-parser.test.ts:480 runs on every bun test invocation of this file, seven times per run via test.each. Today it exits in about 2 ms because NodeHTTPParser.cpp:278 and :358 set m_lastMessageStart and NodeHTTPParser.cpp:325 computes uv_hrtime() minus that value. The dismissal assumed the preceding idle: [] assertion at :475-:478 is a sufficient guard, but that assertion only proves lastMessageStart is non-zero at that instant; it does not bound the loop if duration() later returns a constant (for example a regression that returns 0 for parsers in a list, or reads a cached value). Being synchronous, no setTimeout-based test timeout can interrupt it, so the file hangs until the CI job limit. Root CLAUDE.md says to poll with a deadline. Remedy: add a deadline to the poll and throw with the observed durations when it is crossed.

Verification: nit — triggers only if a future regression makes HTTPParser::duration() stop advancing. test/js/node/http/node-http-parser.test.ts:480 is a synchronous spin with no wall-clock bound. Per-test timeout cannot fire: bun test's timeout is enforced by the event loop (src/runtime/test_runner/Execution.rs:298-335). So a sync spin turns a would-be assertion failure into a hung bun test process.


const names = new Map([
[incomplete, "incomplete"],
[complete, "complete"],
]);
expect(list.expired(headersTimeout, requestTimeout).map(parser => names.get(parser))).toEqual(expired);
});
});

test("subclasses of HTTPParser and ConnectionsList return instances of the subclass", () => {
Expand Down
Loading