node
node copied to clipboard
nodejs
http: resume kept-alive when no body allowed
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 nodejs/node#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.
Setting to draft, went to fix review nit and add tests and I am not convinced issue is fully resolved.
@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.
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).
@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?
I think the problem might apply to all expected empty responses. I need to check that for undici as well.
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.
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.
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.
In a twist of irony, my test appears to have left a socket open. Will take a look and push fix.
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 👍