Skip to content

Commit

Permalink
http: resume kept-alive when no body allowed
Browse files Browse the repository at this point in the history
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 socket will be kept-alive, resume the socket during parsing so that it may be returned to the free pool.

Fixes nodejs#47228
  • Loading branch information
mweberxyz committed Apr 5, 2024
1 parent 9efc84a commit 2dee82a
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 2 deletions.
18 changes: 16 additions & 2 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,13 @@ function statusIsInformational(status) {
return (status < 200 && status >= 100 && status !== 101);
}

function responseCannotContainMessageBody(status, method) {
// RFC9112 Section 6.1.1
return method === 'HEAD'
|| status === 204
|| status === 304
}

// client
function parserOnIncomingClient(res, shouldKeepAlive) {
const socket = this.socket;
Expand Down Expand Up @@ -654,11 +661,18 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
return 1; // Skip body but don't treat as Upgrade.
}

if (req.shouldKeepAlive && !shouldKeepAlive && !req.upgradeOrConnect) {
if (req.shouldKeepAlive && !req.upgradeOrConnect) {
// Socket may be kept alive and response cannot contain
// a message body, resume to allow socket to return to free pool
if (responseCannotContainMessageBody(res.statusCode, method)){
res.resume()
}
// Server MUST respond with Connection:keep-alive for us to enable it.
// If we've been upgraded (via WebSockets) we also shouldn't try to
// keep the connection open.
req.shouldKeepAlive = false;
if (!shouldKeepAlive) {
req.shouldKeepAlive = false;
}
}

if (req[kClientRequestStatistics] && hasObserver('http')) {
Expand Down
52 changes: 52 additions & 0 deletions test/parallel/test-http-agent-exit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict';

const common = require('../common');
const cp = require('child_process');
const http = require('http');

if (process.argv[2] === 'server') {
const server = http.createServer((req, res) => {
if(req.method === "HEAD") {
res.writeHead(200);
res.end();
}
else if (req.url === '/204') {
res.writeHead(204);
res.end();
}
else if (req.url === '/304') {
res.writeHead(304);
res.end();
}
});

server.listen(0, () => {
process.send(server.address().port);
});
} else {
const serverProcess = cp.fork(__filename, ['server'], {
stdio: ['ignore', 'ignore', 'ignore', 'ipc']
});
serverProcess.once('message', common.mustCall((port) => {
serverProcess.channel.unref();
serverProcess.unref();
const agent = new http.Agent({ keepAlive: true });

// Make requests without consuming response
http.get({ method: "HEAD", host: common.localhostIPv4, port, agent }, common.mustCall());
http.get({ method: "GET", host: common.localhostIPv4, port, agent, path: '/204' }, common.mustCall());
http.get({ method: "GET", host: common.localhostIPv4, port, agent, path: '/304' }, common.mustCall());

// Ensure handlers are called/not called as expected
const cb = (res) => {
res.on('end', common.mustCall())
res.on('data', common.mustNotCall())
}
http.get({ method: "HEAD", host: common.localhostIPv4, port, agent }, cb);
http.get({ method: "GET", host: common.localhostIPv4, port, agent, path: '/204' }, cb);
http.get({ method: "GET", host: common.localhostIPv4, port, agent, path: '/304' }, cb);
}));

// HEAD, 204, and 304 requests should not block script exit
setTimeout(common.mustNotCall(), common.platformTimeout(3000)).unref();
}

0 comments on commit 2dee82a

Please sign in to comment.