http: resume kept-alive when no body allowed #52329

mweberxyz · 2024-04-02T17:41:06Z

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 · 2024-04-02T17:41:11Z

Review requested:

@nodejs/http
@nodejs/net

ShogunPanda

Nit. Rest LGTM.

ShogunPanda · 2024-04-05T06:47:38Z

lib/_http_client.js

+    if (!shouldKeepAlive) {
+      req.shouldKeepAlive = false;
+    }
+


Please remove this space.

mweberxyz · 2024-04-05T15:54:50Z

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

mweberxyz · 2024-04-05T17:54:10Z

@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 · 2024-04-09T10:21:08Z

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 · 2024-04-09T16:11:00Z

@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 · 2024-04-09T16:16:13Z

What does this PR do then?

ronag · 2024-04-09T16:17:02Z

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

mweberxyz · 2024-04-09T16:19:25Z

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 · 2024-04-09T16:22:36Z

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 · 2024-04-09T16:26:36Z

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 · 2024-04-09T18:49:27Z

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

mweberxyz · 2024-04-09T20:04:25Z

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

ShogunPanda

LGTM!

mweberxyz · 2024-04-10T15:11:41Z

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 · 2024-10-15T16:47:52Z

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

mcollina · 2024-12-07T04:00:33Z

@mweberxyz would you mind to rebase this on top of main? Thanks. We should try to get this landed.

hui-an-yang · 2025-01-15T23:53:29Z

Hello all

Would like to check if there is any update on this?
Still having socket hang up error with node 19 and above

According to RFC9112 section 6.3.1: HEAD requests, and responses with status 204 and 304 cannot contain a message body, If a socket will be kept-alive, resume the socket during parsing so that it may be returned to the free pool. Fixes nodejs#47228

mweberxyz · 2025-01-24T17:54:42Z

@mcollina ready to go

nodejs-github-bot · 2025-02-03T09:29:31Z

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

codecov · 2025-02-03T11:24:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (c752615) to head (e21ef89).
Report is 565 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #52329      +/-   ##
==========================================
- Coverage   89.21%   89.20%   -0.01%     
==========================================
  Files         662      662              
  Lines      191976   191990      +14     
  Branches    36950    36957       +7     
==========================================
+ Hits       171271   171273       +2     
- Misses      13545    13552       +7     
- Partials     7160     7165       +5

Files with missing lines	Coverage Δ
lib/_http_client.js	`98.01% <100.00%> (+0.02%)`	⬆️

... and 29 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.

mcollina

lgtm

nodejs-github-bot · 2025-03-12T14:56:16Z

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, needs-ci
Commits    1
 - http: resume kept-alive when no body allowed
Committers 1
 - Matt Weber <hello@mweber.xyz>
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>
------------------------------ 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>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 02 Apr 2024 17:41:06 GMT
   ✔  Approvals: 4
   ✔  - 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-2632844122
   ✔  - 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
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2025-02-03T09:29:31Z: https://ci.nodejs.org/job/node-test-pull-request/64942/
- Querying data for job/node-test-pull-request/64942/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

mcollina

lgtm

nodejs-github-bot · 2025-04-01T18:18:35Z

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

nodejs-github-bot · 2025-04-05T15:41:36Z

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

jasnell · 2025-04-05T15:41:49Z

test/parallel/test-http-agent-exit.js

+      res.end();
+    }
+  });
+


Nit: A comment explaining what this test is asserting for would be helpful.

nodejs-github-bot · 2025-04-05T17:58:25Z

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

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Apr 2, 2024

ShogunPanda approved these changes Apr 5, 2024

View reviewed changes

lib/_http_client.js Outdated

if (!shouldKeepAlive) {

req.shouldKeepAlive = false;

}

Copy link

Contributor

ShogunPanda Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this space.

mweberxyz marked this pull request as draft April 5, 2024 15:53

mweberxyz force-pushed the http-keep-alive-auto-resume branch from 2de97af to be6b085 Compare April 5, 2024 17:52

mweberxyz marked this pull request as ready for review April 5, 2024 17:52

mweberxyz requested a review from ShogunPanda April 5, 2024 17:55

mweberxyz force-pushed the http-keep-alive-auto-resume branch from be6b085 to 2dee82a Compare April 5, 2024 19:06

mcollina approved these changes Apr 9, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2024

ronag approved these changes Apr 9, 2024

View reviewed changes

mweberxyz force-pushed the http-keep-alive-auto-resume branch from 2dee82a to 8c4db34 Compare April 9, 2024 16:41

mweberxyz force-pushed the http-keep-alive-auto-resume branch from 8c4db34 to cf6677a Compare April 9, 2024 20:03

ShogunPanda approved these changes Apr 10, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024

This comment was marked as outdated.

Sign in to view

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024

mweberxyz force-pushed the http-keep-alive-auto-resume branch from 563fed7 to e21ef89 Compare January 24, 2025 16:29

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025

mcollina approved these changes Feb 21, 2025

View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 21, 2025

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 12, 2025

mcollina approved these changes Mar 27, 2025

View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 27, 2025

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 27, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

jasnell reviewed Apr 5, 2025

View reviewed changes

test/parallel/test-http-agent-exit.js

res.end();

}

});

Copy link

Member

jasnell Apr 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: A comment explaining what this test is asserting for would be helpful.

jasnell approved these changes Apr 5, 2025

View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: resume kept-alive when no body allowed #52329

http: resume kept-alive when no body allowed #52329

mweberxyz commented Apr 2, 2024 •

edited

Loading

nodejs-github-bot commented Apr 2, 2024

ShogunPanda left a comment

ShogunPanda Apr 5, 2024

mweberxyz commented Apr 5, 2024

mweberxyz commented Apr 5, 2024

mcollina left a comment

ronag commented Apr 9, 2024 •

edited

Loading

mweberxyz commented Apr 9, 2024

ronag commented Apr 9, 2024

ronag commented Apr 9, 2024

mweberxyz commented Apr 9, 2024

ronag commented Apr 9, 2024

ronag left a comment

mweberxyz commented Apr 9, 2024 •

edited

Loading

mweberxyz commented Apr 9, 2024

mweberxyz commented Apr 9, 2024

ShogunPanda left a comment

mweberxyz commented Apr 10, 2024

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Oct 15, 2024

mcollina commented Dec 7, 2024

hui-an-yang commented Jan 15, 2025

mweberxyz commented Jan 24, 2025

nodejs-github-bot commented Feb 3, 2025

codecov bot commented Feb 3, 2025 •

edited

Loading

mcollina left a comment

nodejs-github-bot commented Mar 12, 2025

mcollina left a comment

nodejs-github-bot commented Apr 1, 2025

nodejs-github-bot commented Apr 5, 2025

jasnell Apr 5, 2025

nodejs-github-bot commented Apr 5, 2025

http: resume kept-alive when no body allowed #52329

Are you sure you want to change the base?

http: resume kept-alive when no body allowed #52329

Conversation

mweberxyz commented Apr 2, 2024 • edited Loading

nodejs-github-bot commented Apr 2, 2024

ShogunPanda left a comment

Choose a reason for hiding this comment

ShogunPanda Apr 5, 2024

Choose a reason for hiding this comment

mweberxyz commented Apr 5, 2024

mweberxyz commented Apr 5, 2024

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Apr 9, 2024 • edited Loading

mweberxyz commented Apr 9, 2024

ronag commented Apr 9, 2024

ronag commented Apr 9, 2024

mweberxyz commented Apr 9, 2024

ronag commented Apr 9, 2024

ronag left a comment

Choose a reason for hiding this comment

mweberxyz commented Apr 9, 2024 • edited Loading

mweberxyz commented Apr 9, 2024

mweberxyz commented Apr 9, 2024

ShogunPanda left a comment

Choose a reason for hiding this comment

mweberxyz commented Apr 10, 2024

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Oct 15, 2024

mcollina commented Dec 7, 2024

hui-an-yang commented Jan 15, 2025

mweberxyz commented Jan 24, 2025

nodejs-github-bot commented Feb 3, 2025

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 12, 2025

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 1, 2025

nodejs-github-bot commented Apr 5, 2025

jasnell Apr 5, 2025

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 5, 2025

mweberxyz commented Apr 2, 2024 •

edited

Loading

ronag commented Apr 9, 2024 •

edited

Loading

mweberxyz commented Apr 9, 2024 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading