http: resume kept-alive when no body allowed

[mweberxyz]

In accordance with https://www.rfc-editor.org/rfc/rfc9112#name-message-body-length: HEAD, 1xx, 204, and 304 responses cannot contain a message body.

If a connection will be kept-alive, resume the socket during parsing so that it may be returned to the free pool, even if the caller has not consumed the known-expected-empty response.

Fixes: #47228

Note: The original source of the issue reporter's issue was due to failure to consume the response body of a 3xx request, which this change will not resolve. As an option, 3xx with a Content-Length of 0 could receive the same auto-resume treatment. I am hesitant to propose that change, as some HTTP servers may return a message body (ie <a href=here>Moved Here</a>), which would then make the caller's responsibility to consume or resume dependent on the particular remote server's behavior.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[ShogunPanda]

Nit. Rest LGTM.

[ShogunPanda]

Please remove this space.

[mweberxyz]

Setting to draft, went to fix review nit and add tests and I am not convinced issue is fully resolved.

[mweberxyz]

@ShogunPanda I added tests and discovered that my approach putting the logic in in an else if following the !shouldKeepAlive condition will work for 204 and 304s, but not for HEAD, because at this point shouldKeepAlive is set to false for HEAD requests -- this is coming from llhttp.

[mcollina]

lgtm

[ronag]

This is potentially unsafe. Some servers don't properly end HEAD requests and there is no way to detect it. Such a case would potentially corrupt following responses from the same connection. Which is why in undici we always reset the connection after a HEAD request, unless explicitly told otherwise (reset: false option).

[mweberxyz]

@ronag re-using a socket following a HEAD request is the existing logic.

I checked with the following replication:

const http = require("http");

const noKeepalive = process.argv.at(-1) === "no-keepalive";

const opts = {};

if (noKeepalive) {
  opts.agent = new http.Agent({ keepAlive: false });
}

console.log({
  version: process.version,
  keepAlive: !noKeepalive,
});

http.get("http://www.example.com", { method: "HEAD", ...opts }, (res) => {
  res.resume();
  const headSocket = res.socket;
  setTimeout(() => {
    http.get("http://www.example.com", { method: "GET", ...opts }, (res2) => {
      console.log({ sameSocket: headSocket === res2.socket });
    });
  }, 500);
});

gives:

% node index.js keepalive
{ version: 'v20.12.1', keepAlive: true }
{ sameSocket: true }

and

% node index.js no-keepalive
{ version: 'v20.12.1', keepAlive: false }
{ sameSocket: false }

Should I take a look at that first before returning to this PR?

[ronag]

What does this PR do then?

[ronag]

I think the problem might apply to all expected empty responses. I need to check that for undici as well.

[mweberxyz]

What does this PR do then?

Makes it so the res.resume() following a HEAD request, a request that returns a 204, or a request that returns a 304 is implicit.

[ronag]

Makes it so the res.resume() following a HEAD request, a request that returns a 204, or a request that returns a 304 is implicit.

Sorry, I don't understand.

[ronag]

Now I get it. LGTM.

[mweberxyz]

FWIW what lead me here is a package that was using node:https and not consuming the response on a request that returned a 302, which prevented node from exiting until the keepAlive timed out. I can't fix that because a 302 could return a response, so it should be up to the caller to avoid keeping node from exiting, but figured in these three cases where an http request could not possibly return a response, per spec, we can safely do this automatically.

Will push changes for lint errors shortly.

[mweberxyz]

In a twist of irony, my test appears to have left a socket open. Will take a look and push fix.

[mweberxyz]

Ready for ci: the server in the test needed to be unref'd.

[ShogunPanda]

LGTM!

[mweberxyz]

I can't replicate that final test-asan failure locally - let me know if there's anything else I need to do to get this ready to land 👍

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/52329
✔  Done loading data for nodejs/node/pull/52329
----------------------------------- PR info ------------------------------------
Title      http: resume kept-alive when no body allowed (#52329)
Author     Matt Weber <hello@mweber.xyz> (@mweberxyz, first-time contributor)
Branch     mweberxyz:http-keep-alive-auto-resume -> nodejs:main
Labels     http, semver-minor, author ready, needs-ci
Commits    1
 - http: resume kept-alive when no body allowed
Committers 1
 - Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/52329
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52329
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 02 Apr 2024 17:41:06 GMT
   ✔  Approvals: 5
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-1991621837
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-2722000587
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-1989549614
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/52329#pullrequestreview-2228985328
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-2744925503
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-07-11T07:24:00Z: https://ci.nodejs.org/job/node-test-pull-request/67928/
- Querying data for job/node-test-pull-request/67928/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @mweberxyz(hello@mweber.xyz)
   ⚠  - commit a38c7aeac7f6 is authored by 1062734+mweberxyz@users.noreply.github.com
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/16581371844

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Ethan-Arrowood]

Sorry to see this stall out. Can you rebase and see what might be failing in CI? Would love to help you land this!