Incorrect HTTP keep-alive default when Connection header is removed

[pimterry]

Version

v19.4.0 and v16.18.1 (likely all modern versions)

Platform

Linux zx81 5.19.0-26-generic #27-Ubuntu SMP PREEMPT_DYNAMIC Wed Nov 23 20:44:15 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

http

What steps will reproduce the bug?

const http = require('http');

server = http.createServer((req, res) => {
    console.log('Client port:', req.socket.remotePort);

    res.removeHeader('connection'); // <-- The problem

    res.writeHead(200);
    res.end('Goodbye');
});

server.listen(9090, () => {
    const agent = new http.Agent({ keepAlive: true });
    http.get('http://localhost:9090', { agent }, (res) => {
        res.resume();
        res.on('close', () => {
            console.log('First response completed');

            setTimeout(() => {
                http.get('http://localhost:9090', { agent }, () => {
                    console.log('Got second response');
                    process.exit(0);
                });
            }, 100);
        });
    })
});

This script creates a simple HTTP server that attempts to send a response without sending a Connection header. This response header is not specifically required by any HTTP standard.

How often does it reproduce? Is there a required condition?

Reproduces reliably

What is the expected behavior?

The above should persist the connection between requests - i.e. the remotePort that's logged should always be the same for both requests

What do you see instead?

The client port is different every time, because removing the Connection header incorrectly disable connection persistence.

If the removeHeader line is commented out, the port is indeed the same for both requests.

Additional information

Including a Connection header in responses is perhaps polite, but it's not required or even (AFAICT) encouraged by the spec.

When it is omitted, the connection should still be persistent by default for any HTTP/1.1 request.

I think this is pretty clear in the RFC: https://www.rfc-editor.org/rfc/rfc7230#section-6.3

A recipient determines whether a connection is persistent or not based on the most recently received message's protocol version and Connection header field (if any):

• If the "close" connection option is present, the connection will not persist after the current response; else,
• If the received protocol is HTTP/1.1 (or later), the connection will persist after the current response; else,
  ...

I.e. if no Connection header is returned, for an HTTP/1.1 connection, the connection is expected to persist after the current response.

In Node however, when you remove the Connection header then http.Server always explicitly disables keep-alive for the connection, here:

node/lib/_http_outgoing.js

Lines 508 to 510 in e487638

	if (this._removedConnection) {
	this._last = true;
	this.shouldKeepAlive = false;

This isn't strict MUST behaviour in the RFC - servers are of course allowed to close connections if they need to - but it is against the generally expected HTTP/1.1 keep-alive behaviour and the various SHOULDs here. Servers are expected to persist connections where possible, unless they've explicitly returned a "Connection: close" response.

This won't affect most Node users, since I assume most people don't manually remove default headers like this, but in some cases directly controlling the headers is very useful, e.g. to precisely match responses sent by other systems.

Removing this header is actively supported, and so the resulting behaviour should follow the spec: the response should not have a Connection header, but it should be persisted.

---

As an aside: if the setTimeout is removed from the above example, then it also fails on the client side in the 2nd request, with a socket hang up.

Is that a bug too? Unclear - the server really is unexpectedly closing the connection here, but OTOH it's actually happening before the 2nd request is ever sent, and so the agent shouldn't really be trying to reuse the socket in that case (or maybe it should be retrying automatically, since it's effectively taking a socket from the pool and discovering it's already been closed?).

If there's any setTimeout step (even 0) then the closure seems to be correctly handled by the agent and a separate connection is created.

[bnoordhuis]

For historic background, that behavior was introduced in commit 5555318 from 2012 and I think (but am not 100% sure) it was unintentional.

You're right the Connection header isn't required for http/1.1 (http/1.0 is another story) but it's possible parts of the ecosystem rely on this behavior because it's been around so long.

If you send a pull request, we can run citgm to gauge what the fallout is.

[pimterry]

PR opened: #46331