http: close connections that go idle after server.close()

doc/api/http.md

@@ -1759,6 +1759,11 @@ unless the user specified another socket type.
 <!-- YAML
 added: v0.1.90
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/66000
+    description: Connections that are still active when the method is called
+                 now close once they go idle, instead of staying open until
+                 `server.keepAliveTimeout` elapses.
   - version:
       - v19.0.0
     pr-url: https://github.com/nodejs/node/pull/43522
@@ -1770,7 +1775,9 @@ changes:
 
 Stops the server from accepting new connections and closes all connections
 connected to this server which are not sending a request or waiting for
-a response.
+a response. A connection that is still sending a request or waiting for a
+response is left alone until that request/response finishes, and is then
+closed as well.
 See [`net.Server.close()`][].
 
 ```js

lib/_http_outgoing.js

@@ -38,7 +38,12 @@ const { getDefaultHighWaterMark } = require('internal/streams/state');
 const assert = require('internal/assert');
 const EE = require('events');
 const Stream = require('stream');
-const { kOutHeaders, utcDate, kNeedDrain } = require('internal/http');
+const {
+  kOutHeaders,
+  utcDate,
+  kNeedDrain,
+  kServerClosing,
+} = require('internal/http');
 const { Buffer } = require('buffer');
 const {
   _checkIsHttpToken: checkIsHttpToken,
@@ -536,7 +541,10 @@ function _storeHeader(firstLine, headers) {
     // even if the connection header isn't sent, we still persist by default.
     this._last = !this.shouldKeepAlive;
   } else if (!state.connection) {
+    // A response sent by a server that is shutting down must not advertise
+    // keep-alive: the connection closes as soon as this response finishes.
     const shouldSendKeepAlive = this.shouldKeepAlive &&
+        !this[kSocket]?.server?.[kServerClosing] &&
         (state.contLen || this.useChunkedEncodingByDefault || this.agent);
     if (shouldSendKeepAlive && this.maxRequestsOnConnectionReached) {
       header += 'Connection: close\r\n';

lib/_http_server.js

@@ -61,6 +61,7 @@ const {
 const {
   kOutHeaders,
   kNeedDrain,
+  kServerClosing,
   isTraceHTTPEnabled,
   traceBegin,
   traceEnd,
@@ -620,6 +621,10 @@ function setupConnectionsTracking() {
   // Start connection handling
   this[kConnections] ||= new ConnectionsList();
 
+  // A server can be listened on again after having been closed, so clear the
+  // shutdown mark left by a previous `close()`.
+  this[kServerClosing] = false;
+
   if (this[kConnectionsCheckingInterval]) {
     clearInterval(this[kConnectionsCheckingInterval]);
   }
@@ -630,6 +635,9 @@ function setupConnectionsTracking() {
 }
 
 function httpServerPreClose(server) {
+  // The sweep below only closes the connections that are idle now,
+  // the mark closes the busy ones as soon as they go idle.
+  server[kServerClosing] = true;
   server.closeIdleConnections();
   clearInterval(server[kConnectionsCheckingInterval]);
 }
@@ -1228,7 +1236,7 @@ function resOnFinish(req, res, socket, state, server) {
   clearIncoming(req);
   process.nextTick(emitCloseNT, res);
 
-  if (res._last) {
+  if (res._last || (server[kServerClosing] && state.outgoing.length === 0)) {
     if (typeof socket.destroySoon === 'function') {
       socket.destroySoon();
     } else {

lib/internal/http.js

@@ -268,6 +268,7 @@ function getGlobalAgent(proxyEnv, Agent) {
 module.exports = {
   kOutHeaders: Symbol('kOutHeaders'),
   kNeedDrain: Symbol('kNeedDrain'),
+  kServerClosing: Symbol('kServerClosing'),
   kProxyConfig: Symbol('kProxyConfig'),
   kWaitForProxyTunnel: Symbol('kWaitForProxyTunnel'),
   checkShouldUseProxy,

test/parallel/test-http-raw-headers.js

@@ -93,15 +93,15 @@ http.createServer(common.mustCall(function(req, res) {
       'Date',
       null,
       'Connection',
-      'keep-alive',
+      'close',
       'Transfer-Encoding',
       'chunked',
     ];
     const expectHeaders = { '__proto__': null,
                             'keep-alive': 'timeout=1',
                             'trailer': 'x-foo',
                             'date': null,
-                            'connection': 'keep-alive',
+                            'connection': 'close',
                             'transfer-encoding': 'chunked' };
     res.rawHeaders[5] = null;
     res.headers.date = null;

test/parallel/test-http-server-close-when-idle.js

@@ -0,0 +1,105 @@
+'use strict';
+
+// This tests that `server.close()` also closes the connections that are busy
+// when it is called, once they go idle. Such a connection used to revert to
+// being a normal keep-alive connection and stay open until `keepAliveTimeout`
+// reaped it, long after the server was closed.
+//
+// Every server below sets `keepAliveTimeout: 0` to disable keep-alive reaping,
+// so `server.close()` is the only thing that can end a connection, and every
+// case fails if the connection is still around a second after its response
+// completed.
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+const net = require('net');
+
+function failUnlessClosedNow(server, message) {
+  const timer = setTimeout(() => assert.fail(message), common.platformTimeout(1000));
+
+  // Emitted once the listening handle and every connection are gone.
+  server.on('close', common.mustCall(() => clearTimeout(timer)));
+}
+
+// `close()` while the request is still being handled. The connection is
+// active, so the `closeIdleConnections()` inside `close()` skips it.
+{
+  const server = http.createServer({ keepAliveTimeout: 0 }, common.mustCall((req, res) => {
+    server.close();
+    res.end('ok');
+    failUnlessClosedNow(server, 'connection closed during a request stayed open');
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    http.get({ port: server.address().port }, common.mustCall((res) => {
+      assert.strictEqual(res.headers.connection, 'close');
+      res.resume();
+    }));
+  }));
+}
+
+// `close()` once the request has been received and the handler has returned,
+// with the response still pending. The connection is neither idle nor mid-request at that moment.
+{
+  const server = http.createServer({ keepAliveTimeout: 0 }, common.mustCall((req, res) => {
+    setTimeout(common.mustCall(() => {
+      server.close();
+      res.end('ok');
+      failUnlessClosedNow(server, 'connection closed between request and response stayed open');
+    }), common.platformTimeout(50));
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    http.get({ port: server.address().port }, common.mustCall((res) => {
+      assert.strictEqual(res.headers.connection, 'close');
+      res.resume();
+    }));
+  }));
+}
+
+// The server closes the connection itself rather than leaving that to the
+// client: a raw socket ignores `Connection: close` and is still sent a FIN.
+{
+  const server = http.createServer({ keepAliveTimeout: 0 }, common.mustCall((req, res) => {
+    server.close();
+    res.end('ok');
+    failUnlessClosedNow(server, 'connection of a client ignoring `Connection: close` stayed open');
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    const socket = net.connect(server.address().port, common.mustCall(() => {
+      socket.write('GET / HTTP/1.1\r\nHost: localhost\r\nConnection: keep-alive\r\n\r\n');
+      socket.resume();
+      socket.on('end', common.mustCall(() => socket.end()));
+    }));
+  }));
+}
+
+// `close()` after the response headers have gone out. They advertised
+// keep-alive, which cannot be taken back once written, so only the check made
+// when the response finishes can close this connection.
+{
+  const server = http.createServer({ keepAliveTimeout: 0 }, common.mustCall((req, res) => {
+    res.writeHead(200, { 'Content-Length': 2 });
+    res.write('o');
+    res.flushHeaders();
+
+    setTimeout(common.mustCall(() => {
+      server.close();
+      res.end('k');
+      failUnlessClosedNow(server, 'connection closed after its response headers stayed open');
+    }), common.platformTimeout(50));
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    http.get({ port: server.address().port }, common.mustCall((res) => {
+      assert.strictEqual(res.headers.connection, 'keep-alive');
+
+      res.setEncoding('utf8');
+      let body = '';
+      res.on('data', (chunk) => body += chunk);
+      res.on('end', common.mustCall(() => assert.strictEqual(body, 'ok')));
+    }));
+  }));
+}

test/parallel/test-http-server-unconsume.js

@@ -15,8 +15,6 @@ for (const testFn of testCases) {
     req.socket[testFn]('data', function(data) {
       received += data;
     });
-
-    server.close();
   }).listen(0, common.mustCall(function() {
     const socket = net.connect(this.address().port, common.mustCall(() => {
       socket.write('PUT / HTTP/1.1\r\nHost: example.com\r\n\r\n');
@@ -28,6 +26,7 @@ for (const testFn of testCases) {
       socket.on('end', common.mustCall(() => {
         assert.strictEqual(received, 'hello world',
                            `failed for socket.${testFn}`);
+        server.close();
       }));
     }));
   }));