https: fix connection checking interval not clearing on server close

The connection interval should close when httpsServer.close is called similarly to how it gets cleared when httpServer.close is called. fixes: nodejs#48373 PR-URL: nodejs#48383 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Distributive-Network · Sep 13, 2023 · 4fd8cd6 · 4fd8cd6
1 parent ef85828
commit 4fd8cd6
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 3 deletions.
diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -508,6 +508,11 @@ function setupConnectionsTracking(server) {
     setInterval(checkConnections.bind(server), server.connectionsCheckingInterval).unref();
 }
 
+function httpServerPreClose(server) {
+  server.closeIdleConnections();
+  clearInterval(server[kConnectionsCheckingInterval]);
+}
+
 function Server(options, requestListener) {
   if (!(this instanceof Server)) return new Server(options, requestListener);
 
@@ -549,7 +554,7 @@ ObjectSetPrototypeOf(Server.prototype, net.Server.prototype);
 ObjectSetPrototypeOf(Server, net.Server);
 
 Server.prototype.close = function() {
-  clearInterval(this[kConnectionsCheckingInterval]);
+  httpServerPreClose(this);
   ReflectApply(net.Server.prototype.close, this, arguments);
 };
 
@@ -1188,4 +1193,6 @@ module.exports = {
   storeHTTPOptions,
   _connectionListener: connectionListener,
   kServerResponse,
+  httpServerPreClose,
+  kConnectionsCheckingInterval,
 };
diff --git a/lib/https.js b/lib/https.js
@@ -31,6 +31,7 @@ const {
   JSONStringify,
   ObjectAssign,
   ObjectSetPrototypeOf,
+  ReflectApply,
   ReflectConstruct,
 } = primordials;
 
@@ -43,6 +44,7 @@ assertCrypto();
 const tls = require('tls');
 const { Agent: HttpAgent } = require('_http_agent');
 const {
+  httpServerPreClose,
   Server: HttpServer,
   setupConnectionsTracking,
   storeHTTPOptions,
@@ -97,6 +99,11 @@ Server.prototype.closeIdleConnections = HttpServer.prototype.closeIdleConnection
 
 Server.prototype.setTimeout = HttpServer.prototype.setTimeout;
 
+Server.prototype.close = function() {
+  httpServerPreClose(this);
+  ReflectApply(tls.Server.prototype.close, this, arguments);
+};
+
 /**
  * Creates a new `https.Server` instance.
  * @param {{

diff --git a/test/parallel/test-http-server-close-destroy-timeout.js b/test/parallel/test-http-server-close-destroy-timeout.js
@@ -0,0 +1,13 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { createServer } = require('http');
+const { kConnectionsCheckingInterval } = require('_http_server');
+
+const server = createServer(function(req, res) {});
+server.listen(0, common.mustCall(function() {
+  assert.strictEqual(server[kConnectionsCheckingInterval]._destroyed, false);
+  server.close(common.mustCall(() => {
+    assert(server[kConnectionsCheckingInterval]._destroyed);
+  }));
+}));
diff --git a/test/parallel/test-http-server-close-idle.js b/test/parallel/test-http-server-close-idle.js
@@ -42,7 +42,6 @@ server.listen(0, function() {
         assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive'));
         assert.strictEqual(connections, 2);
 
-        server.closeIdleConnections();
         server.close(common.mustCall());
 
         // Check that only the idle connection got closed

diff --git a/test/parallel/test-https-server-close-destroy-timeout.js b/test/parallel/test-https-server-close-destroy-timeout.js
@@ -0,0 +1,24 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+
+const { createServer } = require('https');
+const { kConnectionsCheckingInterval } = require('_http_server');
+
+const fixtures = require('../common/fixtures');
+
+const options = {
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem')
+};
+
+const server = createServer(options, function(req, res) {});
+server.listen(0, common.mustCall(function() {
+  assert.strictEqual(server[kConnectionsCheckingInterval]._destroyed, false);
+  server.close(common.mustCall(() => {
+    assert(server[kConnectionsCheckingInterval]._destroyed);
+  }));
+}));
diff --git a/test/parallel/test-https-server-close-idle.js b/test/parallel/test-https-server-close-idle.js
@@ -52,7 +52,6 @@ server.listen(0, function() {
         assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive'));
         assert.strictEqual(connections, 2);
 
-        server.closeIdleConnections();
         server.close(common.mustCall());
 
         // Check that only the idle connection got closed