http: close pre-request sockets in closeIdleConnections

[semimikoh]

Problem

server.closeIdleConnections() does not close TCP connections that have been accepted but have not yet sent any HTTP data, contradicting its
documented behavior ("Closes all connections connected to this server which are not
sending a request or waiting for a response") and blocking graceful shutdown when peers (e.g. browsers opening speculative connections) hold
sockets open without writing.

Repro:

import{createServer}from'node:http';import{createConnection}from'node:net';constserver=createServer((req,res)=>res.end('ok'));server.listen(3099,'127.0.0.1');awaitnewPromise((r)=>server.once('listening',r));constsock=createConnection(3099,'127.0.0.1');awaitnewPromise((r)=>sock.once('connect',r));awaitnewPromise((r)=>server.once('connection',r));constclosed=newPromise((r)=>server.close(()=>r(performance.now())));constcloseStart=performance.now();server.closeIdleConnections();constt=awaitPromise.race([closed,newPromise((r)=>setTimeout(()=>r(null),2000)),]);console.log(t===null ? 'still hung after 2s' : `closed in ${t-closeStart}ms`);sock.destroy();

Before: still hung after 2s. After: closes immediately.

Reported in #63452.

Cause

New sockets are pushed into kConnections from connectionListenerInternal via parser.initialize(...), but Parser::Initialize
(src/node_http_parser.cc) sets last_message_start_ = uv_hrtime() so that headersTimeout / requestTimeout can begin counting (DoS
protection). ConnectionsList::Idle() only returns parsers with last_message_start_ === 0, which becomes true only after on_message_complete
resets it. Sockets that were accepted but have not sent any HTTP data are therefore classified as active and skipped by closeIdleConnections().

Fix

src/node_http_parser.cc: add a received_data_ flag on Parser. It is reset to false in Parser::Initialize (for server-side parsers that own
a ConnectionsList) and set to true in on_message_begin, when llhttp signals the first byte of a new HTTP message. ConnectionsList::Idle()
now also returns parsers where received_data_ is still false, so accepted-but-pre-request sockets are reported as idle and closed by
server.closeIdleConnections().

Per @pimterry's review suggestion, the fix lives at the parser layer rather than
as a JS-side filter, so the semantics of idle() are correct for any current or future caller. received_data_ is not part of
ParserComparator's ordering key, so it can be updated in place without pop/push around the std::set.

Test

Added test/parallel/test-http-server-close-idle-connections-pre-request.js. It opens a TCP connection without sending any data, calls
server.close() + server.closeIdleConnections(), and uses an unref'd setTimeout(common.mustNotCall(), 1000) as the safety net: if the fix works
the event loop drains and the timer never fires; without the fix the timer fires and the assertion trips.

Verification

The behavior change was first validated by reproducing the fix's semantics in JS (returning pre-request sockets from idle() equivalent) and
running the issue's repro, which closed in ~0.4ms instead of hanging.

A full local build of the patched binary was not completed because this machine has Apple clang 16.0.0, while the current tree requires a newer
toolchain (V8 fails to compile due to missing std::atomic_ref). The C++ change is small and the semantics are 1:1 with the validated JS-equivalent
behavior; CI will confirm.

Fixes: #63452

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (614050b) to head (700ffb1).
⚠️ Report is 196 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #63470 +/- ##
==========================================
- Coverage 91.67% 90.15% -1.53% 
==========================================
Files 361 718 +357 Lines 156408 227734 +71326 Branches 24050 42756 +18706 ==========================================
+ Hits 143384 205304 +61920 - Misses 12751 14213 +1462 - Partials 273 8217 +7944 

Files with missing lines	Coverage Δ
src/node_http_parser.cc	84.77% <100.00%> (ø)

... and 486 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pimterry]

Thanks for opening this @semimikoh! I'd mildly prefer to do this within ConnectionList itself (see comment) as that'd be a bit cleaner, but I could be persuaded if there's a big issue with that.

[pimterry]

This approach does look like it works fine, but I wonder if it's changing the wrong level. Could we just make idle() return these connections as well? That would be clearer, and avoid hitting the same issue if we use idle() anywhere else.

I think this is fairly doable: I'd suggest adding a boolean field like received_data_ in the parser, set to false in Initalize, and then true in on_message_begin. Single boolean & write, so very cheap, and then ConnectionsList::Idle can return any connections where it's still false.

What do you think?

[semimikoh]

@pimterry
Good point, thanks — moved the fix into Parser / ConnectionsList::Idle as you suggested.

Added a received_data_ flag on Parser, reset to false in Initialize (for server-side parsers that own a ConnectionsList) and set to true in on_message_begin. ConnectionsList::Idle() now returns parsers where received_data_ is still false in addition to the existing last_message_start_ == 0 case, so pre-request sockets show up as idle and closeIdleConnections() closes them with no JS-side workaround.

received_data_ is not part of ParserComparator's ordering key, so updating it in place inside on_message_begin does not require a pop/push around the change.

Force-pushed.

[semimikoh]

@metcoder95 moved the fix from JS to src/node_http_parser.cc per @pimterry's suggestion and force-pushed — mind taking another look? Thanks!

[mcollina]

I'm not convinced. If we have an incoming request with no data, we can assume some will arrive relatively soon, and we can provide a better user experience by handling that rather than killing the connection.

[cyco130]

@mcollina any currently idle connection could send a request soon, whether it has sent one before or not, no?

If the user calls closeIdleConnections, it's a clear signal that they don't want to handle new requests after that point. In the current state of affairs, Chrome's speculative connections block any kind of graceful shutdown (see Fastify 5713 for example).

You can try it: Start a server with Fastify (or with any other framework with graceful shutdown), visit it with Chrome once, then close the page. Then try to shut it down. Chrome's speculative connection will remain open for minutes, blocking the shutdown.

The "ideal" GS sequence in my mind is, very roughly:

1. Stop accepting new connections immediately.
2. Allow in-flight requests to complete (subject to existing request timeout rules).
3. Maybe keep accepting new requests from already open connections until a timeout.
4. Maybe start returning 503 with "Connection: close" for new requests from already open connections after the above timeout (so that a load balancer can handle failover).
5. Close idle connections after the above timeout.
6. Shut down the server after all in-flight requests are handled or timed out.

Steps 4 and/or 5 have to come at some point whether the connection has sent requests before or not. If not, step 6 will close them anyway.

If you still disagree, maybe at least a docs update is in order? Per closeIdleConnections docs:

Closes all connections connected to this server which are not sending a request or waiting for a response.

To me, open connections that have not yet sent a request fit the description. But they are not handled by closeIdleConnections currently.

Added data point: Not necessarily relevant, but Go's server considers pre-request connections as idle when shutting down. So do Deno and Bun, both their native server APIs and node:http emulations.

[mcollina]

lgtm

[gurgunday]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/73862/

[semimikoh]

@pimterry@gurgunday@mcollina
Looks like flaky CI (only osx + openssl32 failed, everything else green). Could someone re-run?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/73915/

[nodejs-github-bot]

Landed in d3a822a

[pimterry]

Thanks for the nudge @semimikoh! Yes, this was just flaky CI, now passing & merged 😄.