From 67817760c8a4846591a309aa979cef9f36752d88 Mon Sep 17 00:00:00 2001 From: Shogun Date: Thu, 28 Apr 2022 12:05:55 +0200 Subject: [PATCH 1/6] http: added connection closing methods --- doc/api/http.md | 17 +++++ doc/api/https.md | 22 +++++- lib/_http_server.js | 16 +++++ lib/https.js | 4 ++ test/parallel/test-http-server-close-all.js | 47 +++++++++++++ test/parallel/test-http-server-close-idle.js | 59 ++++++++++++++++ test/parallel/test-https-server-close-all.js | 58 +++++++++++++++ test/parallel/test-https-server-close-idle.js | 70 +++++++++++++++++++ 8 files changed, 291 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http-server-close-all.js create mode 100644 test/parallel/test-http-server-close-idle.js create mode 100644 test/parallel/test-https-server-close-all.js create mode 100644 test/parallel/test-https-server-close-idle.js diff --git a/doc/api/http.md b/doc/api/http.md index ca845b394b859a..c0ccc2b5a68445 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -1455,6 +1455,23 @@ added: v0.1.90 Stops the server from accepting new connections. See [`net.Server.close()`][]. +### `server.closeAllConnections()` + + + +Closes all connections connected to this server. + +### `server.closeIdleConnections()` + + + +Closes all connections connected to this server which are not sending a request +or waiting for a response. + ### `server.headersTimeout` + +See [`http.Server.closeAllConnections()`][]. + +### `server.closeIdleConnections()` + + + +See [`http.Server.closeIdleConnections()`][]. ### `server.headersTimeout` @@ -523,6 +539,9 @@ headers: max-age=0; pin-sha256="WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; p [`http.Agent(options)`]: http.md#new-agentoptions [`http.Agent`]: http.md#class-httpagent [`http.ClientRequest`]: http.md#class-httpclientrequest +[`http.Server.close()`]: http.md#serverclosecallback +[`http.Server.closeAllConnections()`]: http.md#servercloseallconnections +[`http.Server.closeIdleConnections()`]: http.md#servercloseidleconnections [`http.Server#headersTimeout`]: http.md#serverheaderstimeout [`http.Server#keepAliveTimeout`]: http.md#serverkeepalivetimeout [`http.Server#maxHeadersCount`]: http.md#servermaxheaderscount @@ -530,7 +549,6 @@ headers: max-age=0; pin-sha256="WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; p [`http.Server#setTimeout()`]: http.md#serversettimeoutmsecs-callback [`http.Server#timeout`]: http.md#servertimeout [`http.Server`]: http.md#class-httpserver -[`http.close()`]: http.md#serverclosecallback [`http.createServer()`]: http.md#httpcreateserveroptions-requestlistener [`http.get()`]: http.md#httpgetoptions-callback [`http.request()`]: http.md#httprequestoptions-callback diff --git a/lib/_http_server.js b/lib/_http_server.js index 84be32c78c4075..6b8900bee69304 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -458,6 +458,22 @@ Server.prototype.close = function() { ReflectApply(net.Server.prototype.close, this, arguments); }; +Server.prototype.closeAllConnections = function() { + const connections = this[kConnections].all(); + + for (let i = 0, l = connections.length; i < l; i++) { + connections[i].socket.destroy(); + } +}; + +Server.prototype.closeIdleConnections = function() { + const connections = this[kConnections].idle(); + + for (let i = 0, l = connections.length; i < l; i++) { + connections[i].socket.destroy(); + } +}; + Server.prototype.setTimeout = function setTimeout(msecs, callback) { this.timeout = msecs; if (callback) diff --git a/lib/https.js b/lib/https.js index 74ce93baaafe35..6ca3f335d0f885 100644 --- a/lib/https.js +++ b/lib/https.js @@ -87,6 +87,10 @@ function Server(opts, requestListener) { ObjectSetPrototypeOf(Server.prototype, tls.Server.prototype); ObjectSetPrototypeOf(Server, tls.Server); +Server.prototype.closeAllConnections = HttpServer.prototype.closeAllConnections; + +Server.prototype.closeIdleConnections = HttpServer.prototype.closeIdleConnections; + Server.prototype.setTimeout = HttpServer.prototype.setTimeout; /** diff --git a/test/parallel/test-http-server-close-all.js b/test/parallel/test-http-server-close-all.js new file mode 100644 index 00000000000000..b4b07976f71e34 --- /dev/null +++ b/test/parallel/test-http-server-close-all.js @@ -0,0 +1,47 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const { createServer } = require('http'); +const { connect } = require('net'); + +const server = createServer(common.mustCall(function(req, res) { + res.writeHead(200, { 'Connection': 'keep-alive' }); + res.end(); +}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); + +server.listen(0, function() { + const port = server.address().port; + + // Create a new request, let it finish but leave the connection opened using HTTP keep-alive + const client1 = connect(port); + let response1 = ''; + + client1.on('data', common.mustCall((chunk) => { + response1 += chunk.toString('utf-8'); + })); + + client1.on('end', common.mustCall(function() { + assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + })); + client1.on('close', common.mustCall()); + + client1.resume(); + client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + + // Create a second request but never finish it + const client2 = connect(port); + client2.on('close', common.mustCall()); + + client2.resume(); + client2.write('GET / HTTP/1.1\r\n'); + + // Now, after a while, close the server + setTimeout(common.mustCall(() => { + server.closeAllConnections(); + server.close(common.mustCall()); + }), common.platformTimeout(1000)); + + // This timer should never go off as the server.close should shut everything down + setTimeout(common.mustNotCall(), common.platformTimeout(1500)).unref(); +}); diff --git a/test/parallel/test-http-server-close-idle.js b/test/parallel/test-http-server-close-idle.js new file mode 100644 index 00000000000000..c9c877a19c17e6 --- /dev/null +++ b/test/parallel/test-http-server-close-idle.js @@ -0,0 +1,59 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const { createServer } = require('http'); +const { connect } = require('net'); + +const server = createServer(common.mustCall(function(req, res) { + res.writeHead(200, { 'Connection': 'keep-alive' }); + res.end(); +}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); + +server.listen(0, function() { + const port = server.address().port; + + // Create a new request, let it finish but leave the connection opened using HTTP keep-alive + const client1 = connect(port); + let response1 = ''; + let client1Closed = false; + + client1.on('data', common.mustCall((chunk) => { + response1 += chunk.toString('utf-8'); + })); + + client1.on('end', common.mustCall(function() { + assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + })); + client1.on('close', common.mustCall(() => { + client1Closed = true; + })); + + client1.resume(); + client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + + // Create a second request but never finish it + const client2 = connect(port); + let client2Closed = false; + client2.on('close', common.mustCall(() => { + client2Closed = true; + })); + + client2.resume(); + client2.write('GET / HTTP/1.1\r\n'); + + // Now, after a while, close the server + setTimeout(common.mustCall(() => { + server.closeIdleConnections(); + server.close(common.mustCall()); + }), common.platformTimeout(1000)); + + // Check that only the idle connection got closed + setTimeout(common.mustCall(() => { + assert(client1Closed); + assert(!client2Closed); + + server.closeAllConnections(); + server.close(); + }), common.platformTimeout(1500)).unref(); +}); diff --git a/test/parallel/test-https-server-close-all.js b/test/parallel/test-https-server-close-all.js new file mode 100644 index 00000000000000..e9046377933c87 --- /dev/null +++ b/test/parallel/test-https-server-close-all.js @@ -0,0 +1,58 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); +} + +const { createServer } = require('https'); +const { connect } = require('tls'); + +const fixtures = require('../common/fixtures'); + +const options = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') +}; + +const server = createServer(options, common.mustCall(function(req, res) { + res.writeHead(200, { 'Connection': 'keep-alive' }); + res.end(); +}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); + +server.listen(0, function() { + const port = server.address().port; + + // Create a new request, let it finish but leave the connection opened using HTTP keep-alive + const client1 = connect({ port, rejectUnauthorized: false }); + let response1 = ''; + + client1.on('data', common.mustCall((chunk) => { + response1 += chunk.toString('utf-8'); + })); + + client1.on('end', common.mustCall(function() { + assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + })); + client1.on('close', common.mustCall()); + + client1.resume(); + client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + + // Create a second request but never finish it + const client2 = connect({ port, rejectUnauthorized: false }); + client2.on('close', common.mustCall()); + + client2.resume(); + client2.write('GET / HTTP/1.1\r\n'); + + // Now, after a while, close the server + setTimeout(common.mustCall(() => { + server.closeAllConnections(); + server.close(common.mustCall()); + }), common.platformTimeout(1000)); + + // This timer should never go off as the server.close should shut everything down + setTimeout(common.mustNotCall(), common.platformTimeout(1500)).unref(); +}); diff --git a/test/parallel/test-https-server-close-idle.js b/test/parallel/test-https-server-close-idle.js new file mode 100644 index 00000000000000..a0bf99eee38530 --- /dev/null +++ b/test/parallel/test-https-server-close-idle.js @@ -0,0 +1,70 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); +} + +const { createServer } = require('https'); +const { connect } = require('tls'); + +const fixtures = require('../common/fixtures'); + +const options = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') +}; + +const server = createServer(options, common.mustCall(function(req, res) { + res.writeHead(200, { 'Connection': 'keep-alive' }); + res.end(); +}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); + +server.listen(0, function() { + const port = server.address().port; + + // Create a new request, let it finish but leave the connection opened using HTTP keep-alive + const client1 = connect({ port, rejectUnauthorized: false }); + let response1 = ''; + let client1Closed = false; + + client1.on('data', common.mustCall((chunk) => { + response1 += chunk.toString('utf-8'); + })); + + client1.on('end', common.mustCall(function() { + assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + })); + client1.on('close', common.mustCall(() => { + client1Closed = true; + })); + + client1.resume(); + client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + + // Create a second request but never finish it + const client2 = connect({ port, rejectUnauthorized: false }); + let client2Closed = false; + client2.on('close', common.mustCall(() => { + client2Closed = true; + })); + + client2.resume(); + client2.write('GET / HTTP/1.1\r\n'); + + // Now, after a while, close the server + setTimeout(common.mustCall(() => { + server.closeIdleConnections(); + server.close(common.mustCall()); + }), common.platformTimeout(1000)); + + // Check that only the idle connection got closed + setTimeout(common.mustCall(() => { + assert(client1Closed); + assert(!client2Closed); + + server.closeAllConnections(); + server.close(); + }), common.platformTimeout(1500)).unref(); +}); From d1be124848a200875036af9e90dd3505ffdb33cb Mon Sep 17 00:00:00 2001 From: Shogun Date: Fri, 29 Apr 2022 07:52:40 +0200 Subject: [PATCH 2/6] http: fix set push/pop --- src/node_http_parser.cc | 27 ++++++-- test/parallel/test-http-server-close-all.js | 49 +++++++------- test/parallel/test-http-server-close-idle.js | 67 ++++++++++--------- test/parallel/test-https-server-close-all.js | 49 +++++++------- test/parallel/test-https-server-close-idle.js | 67 ++++++++++--------- 5 files changed, 142 insertions(+), 117 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index c1255b8cbd3b13..7ff5b0fe2ff76f 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -257,9 +257,10 @@ class Parser : public AsyncWrap, public StreamListener { SET_SELF_SIZE(Parser) int on_message_begin() { - // Important: Pop from the list BEFORE resetting the last_message_start_ + // Important: Pop from the lists BEFORE resetting the last_message_start_ // otherwise std::set.erase will fail. if (connectionsList_ != nullptr) { + connectionsList_->Pop(this); connectionsList_->PopActive(this); } @@ -270,6 +271,7 @@ class Parser : public AsyncWrap, public StreamListener { status_message_.Reset(); if (connectionsList_ != nullptr) { + connectionsList_->Push(this); connectionsList_->PushActive(this); } @@ -492,14 +494,19 @@ class Parser : public AsyncWrap, public StreamListener { int on_message_complete() { HandleScope scope(env()->isolate()); - // Important: Pop from the list BEFORE resetting the last_message_start_ + // Important: Pop from the lists BEFORE resetting the last_message_start_ // otherwise std::set.erase will fail. if (connectionsList_ != nullptr) { + connectionsList_->Pop(this); connectionsList_->PopActive(this); } last_message_start_ = 0; + if (connectionsList_ != nullptr) { + connectionsList_->Push(this); + } + if (num_fields_) Flush(); // Flush trailing HTTP headers. @@ -666,12 +673,14 @@ class Parser : public AsyncWrap, public StreamListener { if (connectionsList != nullptr) { parser->connectionsList_ = connectionsList; - parser->connectionsList_->Push(parser); - // This protects from a DoS attack where an attacker establishes // the connection without sending any data on applications where // server.timeout is left to the default value of zero. parser->last_message_start_ = uv_hrtime(); + + // Important: Push into the lists AFTER setting the last_message_start_ + // otherwise std::set.erase will fail later. + parser->connectionsList_->Push(parser); parser->connectionsList_->PushActive(parser); } else { parser->connectionsList_ = nullptr; @@ -1044,10 +1053,14 @@ class Parser : public AsyncWrap, public StreamListener { }; bool ParserComparator::operator()(const Parser* lhs, const Parser* rhs) const { - if (lhs->last_message_start_ == 0) { - return false; - } else if (rhs->last_message_start_ == 0) { + if (lhs->last_message_start_ == 0 && rhs->last_message_start_ == 0) { + // When both parsers are idle, guarantee strict order by + // comparing pointers as ints. + return lhs < rhs; + } else if (lhs->last_message_start_ == 0) { return true; + } else if (rhs->last_message_start_ == 0) { + return false; } return lhs->last_message_start_ < rhs->last_message_start_; diff --git a/test/parallel/test-http-server-close-all.js b/test/parallel/test-http-server-close-all.js index b4b07976f71e34..31ba0aa6691e2e 100644 --- a/test/parallel/test-http-server-close-all.js +++ b/test/parallel/test-http-server-close-all.js @@ -8,40 +8,43 @@ const { connect } = require('net'); const server = createServer(common.mustCall(function(req, res) { res.writeHead(200, { 'Connection': 'keep-alive' }); res.end(); -}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); +}), { + headersTimeout: 0, + keepAliveTimeout: 0, + requestTimeout: common.platformTimeout(60000), +}); server.listen(0, function() { const port = server.address().port; - // Create a new request, let it finish but leave the connection opened using HTTP keep-alive + // Create a first request but never finish it const client1 = connect(port); - let response1 = ''; - - client1.on('data', common.mustCall((chunk) => { - response1 += chunk.toString('utf-8'); - })); - client1.on('end', common.mustCall(function() { - assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); - })); client1.on('close', common.mustCall()); + + client1.on('error', () => {}); - client1.resume(); - client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + client1.write('GET / HTTP/1.1'); - // Create a second request but never finish it + // Create a second request, let it finish but leave the connection opened using HTTP keep-alive const client2 = connect(port); - client2.on('close', common.mustCall()); + let response = ''; - client2.resume(); - client2.write('GET / HTTP/1.1\r\n'); + client2.on('data', common.mustCall((chunk) => { + response += chunk.toString('utf-8'); - // Now, after a while, close the server - setTimeout(common.mustCall(() => { - server.closeAllConnections(); - server.close(common.mustCall()); - }), common.platformTimeout(1000)); + if (response.endsWith('0\r\n\r\n')) { + assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + + server.closeAllConnections(); + server.close(common.mustCall()); + + // This timer should never go off as the server.close should shut everything down + setTimeout(common.mustNotCall(), common.platformTimeout(1500)).unref(); + } + })); + + client2.on('close', common.mustCall()); - // This timer should never go off as the server.close should shut everything down - setTimeout(common.mustNotCall(), common.platformTimeout(1500)).unref(); + client2.write('GET / HTTP/1.1\r\n\r\n'); }); diff --git a/test/parallel/test-http-server-close-idle.js b/test/parallel/test-http-server-close-idle.js index c9c877a19c17e6..ede3dcbeba215e 100644 --- a/test/parallel/test-http-server-close-idle.js +++ b/test/parallel/test-http-server-close-idle.js @@ -8,52 +8,55 @@ const { connect } = require('net'); const server = createServer(common.mustCall(function(req, res) { res.writeHead(200, { 'Connection': 'keep-alive' }); res.end(); -}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); +}), { + headersTimeout: 0, + keepAliveTimeout: 0, + requestTimeout: common.platformTimeout(60000), +}); server.listen(0, function() { const port = server.address().port; - - // Create a new request, let it finish but leave the connection opened using HTTP keep-alive - const client1 = connect(port); - let response1 = ''; let client1Closed = false; + let client2Closed = false; - client1.on('data', common.mustCall((chunk) => { - response1 += chunk.toString('utf-8'); - })); + // Create a first request but never finish it + const client1 = connect(port); - client1.on('end', common.mustCall(function() { - assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); - })); client1.on('close', common.mustCall(() => { client1Closed = true; })); + + client1.on('error', () => {}); - client1.resume(); - client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + client1.write('GET / HTTP/1.1'); - // Create a second request but never finish it + // Create a second request, let it finish but leave the connection opened using HTTP keep-alive const client2 = connect(port); - let client2Closed = false; - client2.on('close', common.mustCall(() => { - client2Closed = true; - })); + let response = ''; + + client2.on('data', common.mustCall((chunk) => { + response += chunk.toString('utf-8'); - client2.resume(); - client2.write('GET / HTTP/1.1\r\n'); + if (response.endsWith('0\r\n\r\n')) { + assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); - // Now, after a while, close the server - setTimeout(common.mustCall(() => { - server.closeIdleConnections(); - server.close(common.mustCall()); - }), common.platformTimeout(1000)); + server.closeIdleConnections(); + server.close(common.mustCall()); - // Check that only the idle connection got closed - setTimeout(common.mustCall(() => { - assert(client1Closed); - assert(!client2Closed); + // Check that only the idle connection got closed + setTimeout(common.mustCall(() => { + assert(!client1Closed); + assert(client2Closed); + + server.closeAllConnections(); + server.close(common.mustCall()); + }), common.platformTimeout(500)).unref(); + } + })); + + client2.on('close', common.mustCall(() => { + client2Closed = true; + })); - server.closeAllConnections(); - server.close(); - }), common.platformTimeout(1500)).unref(); + client2.write('GET / HTTP/1.1\r\n\r\n'); }); diff --git a/test/parallel/test-https-server-close-all.js b/test/parallel/test-https-server-close-all.js index e9046377933c87..62dfacf2a8c2a1 100644 --- a/test/parallel/test-https-server-close-all.js +++ b/test/parallel/test-https-server-close-all.js @@ -19,40 +19,43 @@ const options = { const server = createServer(options, common.mustCall(function(req, res) { res.writeHead(200, { 'Connection': 'keep-alive' }); res.end(); -}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); +}), { + headersTimeout: 0, + keepAliveTimeout: 0, + requestTimeout: common.platformTimeout(60000), +}); server.listen(0, function() { const port = server.address().port; - // Create a new request, let it finish but leave the connection opened using HTTP keep-alive + // Create a first request but never finish it const client1 = connect({ port, rejectUnauthorized: false }); - let response1 = ''; - - client1.on('data', common.mustCall((chunk) => { - response1 += chunk.toString('utf-8'); - })); - client1.on('end', common.mustCall(function() { - assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); - })); client1.on('close', common.mustCall()); + + client1.on('error', () => {}); - client1.resume(); - client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + client1.write('GET / HTTP/1.1'); - // Create a second request but never finish it + // Create a second request, let it finish but leave the connection opened using HTTP keep-alive const client2 = connect({ port, rejectUnauthorized: false }); - client2.on('close', common.mustCall()); + let response = ''; - client2.resume(); - client2.write('GET / HTTP/1.1\r\n'); + client2.on('data', common.mustCall((chunk) => { + response += chunk.toString('utf-8'); - // Now, after a while, close the server - setTimeout(common.mustCall(() => { - server.closeAllConnections(); - server.close(common.mustCall()); - }), common.platformTimeout(1000)); + if (response.endsWith('0\r\n\r\n')) { + assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + + server.closeAllConnections(); + server.close(common.mustCall()); + + // This timer should never go off as the server.close should shut everything down + setTimeout(common.mustNotCall(), common.platformTimeout(1500)).unref(); + } + })); + + client2.on('close', common.mustCall()); - // This timer should never go off as the server.close should shut everything down - setTimeout(common.mustNotCall(), common.platformTimeout(1500)).unref(); + client2.write('GET / HTTP/1.1\r\n\r\n'); }); diff --git a/test/parallel/test-https-server-close-idle.js b/test/parallel/test-https-server-close-idle.js index a0bf99eee38530..725a3036c4affc 100644 --- a/test/parallel/test-https-server-close-idle.js +++ b/test/parallel/test-https-server-close-idle.js @@ -19,52 +19,55 @@ const options = { const server = createServer(options, common.mustCall(function(req, res) { res.writeHead(200, { 'Connection': 'keep-alive' }); res.end(); -}), { requestTimeout: common.platformTimeout(60000), headersTimeout: 0 }); +}), { + headersTimeout: 0, + keepAliveTimeout: 0, + requestTimeout: common.platformTimeout(60000), +}); server.listen(0, function() { const port = server.address().port; - - // Create a new request, let it finish but leave the connection opened using HTTP keep-alive - const client1 = connect({ port, rejectUnauthorized: false }); - let response1 = ''; let client1Closed = false; + let client2Closed = false; - client1.on('data', common.mustCall((chunk) => { - response1 += chunk.toString('utf-8'); - })); + // Create a first request but never finish it + const client1 = connect({ port, rejectUnauthorized: false }); - client1.on('end', common.mustCall(function() { - assert(response1.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); - })); client1.on('close', common.mustCall(() => { client1Closed = true; })); + + client1.on('error', () => {}); - client1.resume(); - client1.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n'); + client1.write('GET / HTTP/1.1'); - // Create a second request but never finish it + // Create a second request, let it finish but leave the connection opened using HTTP keep-alive const client2 = connect({ port, rejectUnauthorized: false }); - let client2Closed = false; - client2.on('close', common.mustCall(() => { - client2Closed = true; - })); + let response = ''; + + client2.on('data', common.mustCall((chunk) => { + response += chunk.toString('utf-8'); - client2.resume(); - client2.write('GET / HTTP/1.1\r\n'); + if (response.endsWith('0\r\n\r\n')) { + assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); - // Now, after a while, close the server - setTimeout(common.mustCall(() => { - server.closeIdleConnections(); - server.close(common.mustCall()); - }), common.platformTimeout(1000)); + server.closeIdleConnections(); + server.close(common.mustCall()); - // Check that only the idle connection got closed - setTimeout(common.mustCall(() => { - assert(client1Closed); - assert(!client2Closed); + // Check that only the idle connection got closed + setTimeout(common.mustCall(() => { + assert(!client1Closed); + assert(client2Closed); + + server.closeAllConnections(); + server.close(common.mustCall()); + }), common.platformTimeout(500)).unref(); + } + })); + + client2.on('close', common.mustCall(() => { + client2Closed = true; + })); - server.closeAllConnections(); - server.close(); - }), common.platformTimeout(1500)).unref(); + client2.write('GET / HTTP/1.1\r\n\r\n'); }); From ed4cf86bb5c9ef0da156636157dbad1c754fa3d6 Mon Sep 17 00:00:00 2001 From: Shogun Date: Fri, 29 Apr 2022 08:39:48 +0200 Subject: [PATCH 3/6] http: assert connections count in test --- test/parallel/test-http-server-close-all.js | 11 +++++++++-- test/parallel/test-http-server-close-idle.js | 11 +++++++++-- test/parallel/test-https-server-close-all.js | 11 +++++++++-- test/parallel/test-https-server-close-idle.js | 11 +++++++++-- 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-http-server-close-all.js b/test/parallel/test-http-server-close-all.js index 31ba0aa6691e2e..79fc5c75b70daf 100644 --- a/test/parallel/test-http-server-close-all.js +++ b/test/parallel/test-http-server-close-all.js @@ -5,8 +5,10 @@ const assert = require('assert'); const { createServer } = require('http'); const { connect } = require('net'); +let connections = 0; + const server = createServer(common.mustCall(function(req, res) { - res.writeHead(200, { 'Connection': 'keep-alive' }); + res.writeHead(200, { Connection: 'keep-alive' }); res.end(); }), { headersTimeout: 0, @@ -14,6 +16,10 @@ const server = createServer(common.mustCall(function(req, res) { requestTimeout: common.platformTimeout(60000), }); +server.on('connection', function() { + connections++; +}); + server.listen(0, function() { const port = server.address().port; @@ -21,7 +27,7 @@ server.listen(0, function() { const client1 = connect(port); client1.on('close', common.mustCall()); - + client1.on('error', () => {}); client1.write('GET / HTTP/1.1'); @@ -35,6 +41,7 @@ server.listen(0, function() { if (response.endsWith('0\r\n\r\n')) { assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + assert.strictEqual(connections, 2); server.closeAllConnections(); server.close(common.mustCall()); diff --git a/test/parallel/test-http-server-close-idle.js b/test/parallel/test-http-server-close-idle.js index ede3dcbeba215e..b9389f1e599c72 100644 --- a/test/parallel/test-http-server-close-idle.js +++ b/test/parallel/test-http-server-close-idle.js @@ -5,8 +5,10 @@ const assert = require('assert'); const { createServer } = require('http'); const { connect } = require('net'); +let connections = 0; + const server = createServer(common.mustCall(function(req, res) { - res.writeHead(200, { 'Connection': 'keep-alive' }); + res.writeHead(200, { Connection: 'keep-alive' }); res.end(); }), { headersTimeout: 0, @@ -14,6 +16,10 @@ const server = createServer(common.mustCall(function(req, res) { requestTimeout: common.platformTimeout(60000), }); +server.on('connection', function() { + connections++; +}); + server.listen(0, function() { const port = server.address().port; let client1Closed = false; @@ -25,7 +31,7 @@ server.listen(0, function() { client1.on('close', common.mustCall(() => { client1Closed = true; })); - + client1.on('error', () => {}); client1.write('GET / HTTP/1.1'); @@ -39,6 +45,7 @@ server.listen(0, function() { if (response.endsWith('0\r\n\r\n')) { assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + assert.strictEqual(connections, 2); server.closeIdleConnections(); server.close(common.mustCall()); diff --git a/test/parallel/test-https-server-close-all.js b/test/parallel/test-https-server-close-all.js index 62dfacf2a8c2a1..408af625d1f773 100644 --- a/test/parallel/test-https-server-close-all.js +++ b/test/parallel/test-https-server-close-all.js @@ -16,8 +16,10 @@ const options = { cert: fixtures.readKey('agent1-cert.pem') }; +let connections = 0; + const server = createServer(options, common.mustCall(function(req, res) { - res.writeHead(200, { 'Connection': 'keep-alive' }); + res.writeHead(200, { Connection: 'keep-alive' }); res.end(); }), { headersTimeout: 0, @@ -25,6 +27,10 @@ const server = createServer(options, common.mustCall(function(req, res) { requestTimeout: common.platformTimeout(60000), }); +server.on('connection', function() { + connections++; +}); + server.listen(0, function() { const port = server.address().port; @@ -32,7 +38,7 @@ server.listen(0, function() { const client1 = connect({ port, rejectUnauthorized: false }); client1.on('close', common.mustCall()); - + client1.on('error', () => {}); client1.write('GET / HTTP/1.1'); @@ -46,6 +52,7 @@ server.listen(0, function() { if (response.endsWith('0\r\n\r\n')) { assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + assert.strictEqual(connections, 2); server.closeAllConnections(); server.close(common.mustCall()); diff --git a/test/parallel/test-https-server-close-idle.js b/test/parallel/test-https-server-close-idle.js index 725a3036c4affc..ea43c4367cb738 100644 --- a/test/parallel/test-https-server-close-idle.js +++ b/test/parallel/test-https-server-close-idle.js @@ -16,8 +16,10 @@ const options = { cert: fixtures.readKey('agent1-cert.pem') }; +let connections = 0; + const server = createServer(options, common.mustCall(function(req, res) { - res.writeHead(200, { 'Connection': 'keep-alive' }); + res.writeHead(200, { Connection: 'keep-alive' }); res.end(); }), { headersTimeout: 0, @@ -25,6 +27,10 @@ const server = createServer(options, common.mustCall(function(req, res) { requestTimeout: common.platformTimeout(60000), }); +server.on('connection', function() { + connections++; +}); + server.listen(0, function() { const port = server.address().port; let client1Closed = false; @@ -36,7 +42,7 @@ server.listen(0, function() { client1.on('close', common.mustCall(() => { client1Closed = true; })); - + client1.on('error', () => {}); client1.write('GET / HTTP/1.1'); @@ -50,6 +56,7 @@ server.listen(0, function() { if (response.endsWith('0\r\n\r\n')) { assert(response.startsWith('HTTP/1.1 200 OK\r\nConnection: keep-alive')); + assert.strictEqual(connections, 2); server.closeIdleConnections(); server.close(common.mustCall()); From 432062ac48da9fe5cdd55112915753a4ed48da3d Mon Sep 17 00:00:00 2001 From: Shogun Date: Fri, 29 Apr 2022 10:21:07 +0200 Subject: [PATCH 4/6] http: linted docs --- doc/api/https.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/https.md b/doc/api/https.md index b2908cd21eb35b..67f667c1ed8c5a 100644 --- a/doc/api/https.md +++ b/doc/api/https.md @@ -539,15 +539,15 @@ headers: max-age=0; pin-sha256="WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; p [`http.Agent(options)`]: http.md#new-agentoptions [`http.Agent`]: http.md#class-httpagent [`http.ClientRequest`]: http.md#class-httpclientrequest -[`http.Server.close()`]: http.md#serverclosecallback -[`http.Server.closeAllConnections()`]: http.md#servercloseallconnections -[`http.Server.closeIdleConnections()`]: http.md#servercloseidleconnections [`http.Server#headersTimeout`]: http.md#serverheaderstimeout [`http.Server#keepAliveTimeout`]: http.md#serverkeepalivetimeout [`http.Server#maxHeadersCount`]: http.md#servermaxheaderscount [`http.Server#requestTimeout`]: http.md#serverrequesttimeout [`http.Server#setTimeout()`]: http.md#serversettimeoutmsecs-callback [`http.Server#timeout`]: http.md#servertimeout +[`http.Server.close()`]: http.md#serverclosecallback +[`http.Server.closeAllConnections()`]: http.md#servercloseallconnections +[`http.Server.closeIdleConnections()`]: http.md#servercloseidleconnections [`http.Server`]: http.md#class-httpserver [`http.createServer()`]: http.md#httpcreateserveroptions-requestlistener [`http.get()`]: http.md#httpgetoptions-callback From 96530af6134a9af4e27d12c7fd04da1767c64573 Mon Sep 17 00:00:00 2001 From: Shogun Date: Mon, 2 May 2022 16:49:20 +0200 Subject: [PATCH 5/6] http: make sure connections checking is started --- lib/_http_server.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 6b8900bee69304..2e817908f3c898 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -409,10 +409,11 @@ function storeHTTPOptions(options) { function setupConnectionsTracking(server) { // Start connection handling server[kConnections] = new ConnectionsList(); - if (server.headersTimeout > 0 || server.requestTimeout > 0) { - server[kConnectionsCheckingInterval] = - setInterval(checkConnections.bind(server), server.connectionsCheckingInterval).unref(); - } + + // This checker is started without checking whether any headersTimeout or requestTimeout is non zero + // otherwise it would not be started if such timeouts are modified after createServer. + server[kConnectionsCheckingInterval] = + setInterval(checkConnections.bind(server), server.connectionsCheckingInterval).unref(); } function Server(options, requestListener) { @@ -505,6 +506,10 @@ Server.prototype[EE.captureRejectionSymbol] = function(err, event, ...args) { }; function checkConnections() { + if (this.headersTimeout === 0 && this.requestTimeout === 0) { + return + } + const expired = this[kConnections].expired(this.headersTimeout, this.requestTimeout); for (let i = 0; i < expired.length; i++) { From 0d07a99902fda99eecdbedb60726a10d01326ad6 Mon Sep 17 00:00:00 2001 From: Shogun Date: Mon, 2 May 2022 20:17:02 +0200 Subject: [PATCH 6/6] http: linted code --- lib/_http_server.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 2e817908f3c898..c8dc22929bfabd 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -507,7 +507,7 @@ Server.prototype[EE.captureRejectionSymbol] = function(err, event, ...args) { function checkConnections() { if (this.headersTimeout === 0 && this.requestTimeout === 0) { - return + return; } const expired = this[kConnections].expired(this.headersTimeout, this.requestTimeout);