http: keep HTTP/1.1 conns alive even if the Connection header is removed

Previously persistence was completed disabled if you removed this
header, which is not correct for modern HTTP, where the header is
optional and all connections should persist by default regardless.

PR-URL: #46331
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

README.md

@@ -346,8 +346,6 @@ For information about the governance of the Node.js project, see
   **Zeyu "Alex" Yang** <<himself65@outlook.com>> (he/him)
 * [iansu](https://github.com/iansu) -
   **Ian Sutherland** <<ian@iansutherland.ca>>
-* [indutny](https://github.com/indutny) -
-  **Fedor Indutny** <<fedor@indutny.com>>
 * [JacksonTian](https://github.com/JacksonTian) -
   **Jackson Tian** <<shyvo1987@gmail.com>>
 * [JakobJingleheimer](https://github.com/JakobJingleheimer) -
@@ -534,6 +532,8 @@ For information about the governance of the Node.js project, see
   **Imran Iqbal** <<imran@imraniqbal.org>>
 * [imyller](https://github.com/imyller) -
   **Ilkka Myller** <<ilkka.myller@nodefield.com>>
+* [indutny](https://github.com/indutny) -
+  **Fedor Indutny** <<fedor@indutny.com>>
 * [isaacs](https://github.com/isaacs) -
   **Isaac Z. Schlueter** <<i@izs.me>>
 * [italoacasas](https://github.com/italoacasas) -

lib/_http_outgoing.js

@@ -506,8 +506,9 @@ function _storeHeader(firstLine, headers) {
 
   // keep-alive logic
   if (this._removedConnection) {
-    this._last = true;
-    this.shouldKeepAlive = false;
+    // shouldKeepAlive is generally true for HTTP/1.1. In that common case,
+    // even if the connection header isn't sent, we still persist by default.
+    this._last = !this.shouldKeepAlive;
   } else if (!state.connection) {
     const shouldSendKeepAlive = this.shouldKeepAlive &&
         (state.contLen || this.useChunkedEncodingByDefault || this.agent);

test/parallel/test-http-remove-connection-header-persists-connection.js

@@ -0,0 +1,72 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+const net = require('net');
+const http = require('http');
+
+const server = http.createServer(function(request, response) {
+  // When the connection header is removed, for HTTP/1.1 the connection should still persist.
+  // For HTTP/1.0, the connection should be closed after the response automatically.
+  response.removeHeader('connection');
+
+  response.end('beep boop\n');
+});
+
+const agent = new http.Agent({ keepAlive: true });
+
+function makeHttp11Request(cb) {
+  http.get({
+    port: server.address().port,
+    agent
+  }, function(res) {
+    const socket = res.socket;
+
+    assert.strictEqual(res.statusCode, 200);
+    assert.strictEqual(res.headers.connection, undefined);
+
+    res.setEncoding('ascii');
+    let response = '';
+    res.on('data', function(chunk) {
+      response += chunk;
+    });
+    res.on('end', function() {
+      assert.strictEqual(response, 'beep boop\n');
+
+      // Wait till next tick to ensure that the socket is returned to the agent before
+      // we continue to the next request
+      process.nextTick(function() {
+        cb(socket);
+      });
+    });
+  });
+}
+
+function makeHttp10Request(cb) {
+  // We have to manually make HTTP/1.0 requests since Node does not allow sending them:
+  const socket = net.connect({ port: server.address().port }, function() {
+    socket.write('GET / HTTP/1.0\r\n' +
+               'Host: localhost:' + server.address().port + '\r\n' +
+                '\r\n');
+    socket.resume(); // Ignore the response itself
+
+    setTimeout(function() {
+      cb(socket);
+    }, 10);
+  });
+}
+
+server.listen(0, function() {
+  makeHttp11Request(function(firstSocket) {
+    makeHttp11Request(function(secondSocket) {
+      // Both HTTP/1.1 requests should have used the same socket:
+      assert.strictEqual(firstSocket, secondSocket);
+
+      makeHttp10Request(function(socket) {
+        // The server should have immediately closed the HTTP/1.0 socket:
+        assert.strictEqual(socket.closed, true);
+        server.close();
+      });
+    });
+  });
+});