From 4267b92604ad78584244488e7f7508a690cb80d0 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Tue, 21 Jun 2022 14:50:55 +0200 Subject: [PATCH] http: use Keep-Alive by default in global agents PR-URL: https://github.com/nodejs/node/pull/43522 Fixes: https://github.com/nodejs/node/issues/37184 Reviewed-By: Matteo Collina Reviewed-By: Antoine du Hamel --- doc/api/http.md | 16 ++++++++++- doc/api/https.md | 5 ++++ lib/_http_agent.js | 23 ++++++++++++++-- lib/_http_server.js | 1 + lib/https.js | 2 +- test/async-hooks/test-graph.http.js | 1 + test/parallel/test-http-agent-no-wait.js | 24 +++++++++++++++++ test/parallel/test-http-client-agent.js | 12 +++++---- ...st-http-client-close-with-default-agent.js | 23 ++++++++++++++++ .../test-http-client-headers-array.js | 3 ++- .../test-http-client-keep-alive-hint.js | 27 +++++++++++++++++++ .../test-http-client-spurious-aborted.js | 2 +- .../test-http-client-timeout-on-connect.js | 5 +++- test/parallel/test-http-content-length.js | 18 ++++++++----- test/parallel/test-http-default-encoding.js | 2 +- test/parallel/test-http-max-headers-count.js | 2 +- ...http-outgoing-message-capture-rejection.js | 3 ++- test/parallel/test-http-raw-headers.js | 14 ++++++---- test/parallel/test-http-request-end.js | 2 +- test/parallel/test-http-should-keep-alive.js | 4 +++ .../test-http-unix-socket-keep-alive.js | 2 +- .../test-https-agent-session-eviction.js | 2 ++ test/parallel/test-https-max-headers-count.js | 2 +- test/parallel/test-stream-destroy.js | 6 +++-- test/parallel/test-tls-over-http-tunnel.js | 2 +- test/parallel/test-tls-set-secure-context.js | 3 ++- test/sequential/test-http-econnrefused.js | 2 +- 27 files changed, 173 insertions(+), 35 deletions(-) create mode 100644 test/parallel/test-http-agent-no-wait.js create mode 100644 test/parallel/test-http-client-close-with-default-agent.js create mode 100644 test/parallel/test-http-client-keep-alive-hint.js diff --git a/doc/api/http.md b/doc/api/http.md index 88f2378421998e..0c9117202498a8 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -1449,11 +1449,20 @@ type other than {net.Socket}. * `callback` {Function} -Stops the server from accepting new connections. See [`net.Server.close()`][]. +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. +See [`net.Server.close()`][]. ### `server.closeAllConnections()` @@ -3214,6 +3223,11 @@ server.listen(8000); * {http.Agent} diff --git a/doc/api/https.md b/doc/api/https.md index bc5c8a246dcdf4..ce8a47fedded29 100644 --- a/doc/api/https.md +++ b/doc/api/https.md @@ -309,6 +309,11 @@ https.get('https://encrypted.google.com/', (res) => { Global instance of [`https.Agent`][] for all HTTPS client requests. diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 791a43c8d5d029..14096361760657 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -31,10 +31,12 @@ const { ArrayPrototypeSplice, FunctionPrototypeCall, NumberIsNaN, + NumberParseInt, ObjectCreate, ObjectKeys, ObjectSetPrototypeOf, ObjectValues, + RegExpPrototypeExec, StringPrototypeIndexOf, StringPrototypeSplit, StringPrototypeStartsWith, @@ -492,7 +494,24 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { socket.setKeepAlive(true, this.keepAliveMsecs); socket.unref(); - const agentTimeout = this.options.timeout || 0; + let agentTimeout = this.options.timeout || 0; + + if (socket._httpMessage?.res) { + const keepAliveHint = socket._httpMessage.res.headers['keep-alive']; + + if (keepAliveHint) { + const hint = RegExpPrototypeExec(/^timeout=(\d+)/, keepAliveHint)?.[1]; + + if (hint) { + const serverHintTimeout = NumberParseInt(hint) * 1000; + + if (serverHintTimeout < agentTimeout) { + agentTimeout = serverHintTimeout; + } + } + } + } + if (socket.timeout !== agentTimeout) { socket.setTimeout(agentTimeout); } @@ -542,5 +561,5 @@ function asyncResetHandle(socket) { module.exports = { Agent, - globalAgent: new Agent() + globalAgent: new Agent({ keepAlive: true, scheduling: 'lifo', timeout: 5000 }) }; diff --git a/lib/_http_server.js b/lib/_http_server.js index c312267aa51e2e..e908b1b1b3d5cf 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -478,6 +478,7 @@ ObjectSetPrototypeOf(Server.prototype, net.Server.prototype); ObjectSetPrototypeOf(Server, net.Server); Server.prototype.close = function() { + this.closeIdleConnections(); clearInterval(this[kConnectionsCheckingInterval]); ReflectApply(net.Server.prototype.close, this, arguments); }; diff --git a/lib/https.js b/lib/https.js index 7ae8ab2987538b..0ecd13b1dd36ed 100644 --- a/lib/https.js +++ b/lib/https.js @@ -331,7 +331,7 @@ Agent.prototype._evictSession = function _evictSession(key) { delete this._sessionCache.map[key]; }; -const globalAgent = new Agent(); +const globalAgent = new Agent({ keepAlive: true, scheduling: 'lifo', timeout: 5000 }); /** * Makes a request to a secure web server. diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js index 0a003427f9e133..62d27fe1b54431 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -12,6 +12,7 @@ const hooks = initHooks(); hooks.enable(); const server = http.createServer(common.mustCall((req, res) => { + res.writeHead(200, { 'Connection': 'close' }); res.end(); server.close(common.mustCall()); })); diff --git a/test/parallel/test-http-agent-no-wait.js b/test/parallel/test-http-agent-no-wait.js new file mode 100644 index 00000000000000..62a381e85ce07c --- /dev/null +++ b/test/parallel/test-http-agent-no-wait.js @@ -0,0 +1,24 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const server = http.createServer(function(req, res) { + res.writeHead(200); + res.end(); +}); + +server.listen(0, common.mustCall(() => { + const req = http.get({ port: server.address().port }, (res) => { + assert.strictEqual(res.statusCode, 200); + + res.resume(); + server.close(); + }); + + req.end(); +})); + +// This timer should never go off as the server will close the socket +setTimeout(common.mustNotCall(), 1000).unref(); diff --git a/test/parallel/test-http-client-agent.js b/test/parallel/test-http-client-agent.js index 77616edd99d2fb..0c348acc4bd68a 100644 --- a/test/parallel/test-http-client-agent.js +++ b/test/parallel/test-http-client-agent.js @@ -27,6 +27,7 @@ const Countdown = require('../common/countdown'); let name; const max = 3; +const agent = new http.Agent(); const server = http.Server(common.mustCall((req, res) => { if (req.url === '/0') { @@ -40,27 +41,28 @@ const server = http.Server(common.mustCall((req, res) => { } }, max)); server.listen(0, common.mustCall(() => { - name = http.globalAgent.getName({ port: server.address().port }); + name = agent.getName({ port: server.address().port }); for (let i = 0; i < max; ++i) request(i); })); const countdown = new Countdown(max, () => { - assert(!(name in http.globalAgent.sockets)); - assert(!(name in http.globalAgent.requests)); + assert(!(name in agent.sockets)); + assert(!(name in agent.requests)); server.close(); }); function request(i) { const req = http.get({ port: server.address().port, - path: `/${i}` + path: `/${i}`, + agent }, function(res) { const socket = req.socket; socket.on('close', common.mustCall(() => { countdown.dec(); if (countdown.remaining > 0) { - assert.strictEqual(http.globalAgent.sockets[name].includes(socket), + assert.strictEqual(agent.sockets[name].includes(socket), false); } })); diff --git a/test/parallel/test-http-client-close-with-default-agent.js b/test/parallel/test-http-client-close-with-default-agent.js new file mode 100644 index 00000000000000..8973f454e37728 --- /dev/null +++ b/test/parallel/test-http-client-close-with-default-agent.js @@ -0,0 +1,23 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const server = http.createServer(function(req, res) { + res.writeHead(200); + res.end(); +}); + +server.listen(0, common.mustCall(() => { + const req = http.get({ port: server.address().port }, (res) => { + assert.strictEqual(res.statusCode, 200); + res.resume(); + server.close(); + }); + + req.end(); +})); + +// This timer should never go off as the server will close the socket +setTimeout(common.mustNotCall(), common.platformTimeout(10000)).unref(); diff --git a/test/parallel/test-http-client-headers-array.js b/test/parallel/test-http-client-headers-array.js index 2665c3a97bcf77..d7e8d39c3c2560 100644 --- a/test/parallel/test-http-client-headers-array.js +++ b/test/parallel/test-http-client-headers-array.js @@ -10,7 +10,7 @@ function execute(options) { const expectHeaders = { 'x-foo': 'boom', 'cookie': 'a=1; b=2; c=3', - 'connection': 'close' + 'connection': 'keep-alive' }; // no Host header when you set headers an array @@ -28,6 +28,7 @@ function execute(options) { assert.deepStrictEqual(req.headers, expectHeaders); + res.writeHead(200, { 'Connection': 'close' }); res.end(); }).listen(0, function() { options = Object.assign(options, { diff --git a/test/parallel/test-http-client-keep-alive-hint.js b/test/parallel/test-http-client-keep-alive-hint.js new file mode 100644 index 00000000000000..2618dfd552f8ab --- /dev/null +++ b/test/parallel/test-http-client-keep-alive-hint.js @@ -0,0 +1,27 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const server = http.createServer( + { keepAliveTimeout: common.platformTimeout(60000) }, + function(req, res) { + req.resume(); + res.writeHead(200, { 'Connection': 'keep-alive', 'Keep-Alive': 'timeout=1' }); + res.end('FOO'); + } +); + +server.listen(0, common.mustCall(() => { + http.get({ port: server.address().port }, (res) => { + assert.strictEqual(res.statusCode, 200); + + res.resume(); + server.close(); + }); +})); + + +// This timer should never go off as the agent will parse the hint and terminate earlier +setTimeout(common.mustNotCall(), common.platformTimeout(3000)).unref(); diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js index 850462dadc76f8..e5f0d41d2096ac 100644 --- a/test/parallel/test-http-client-spurious-aborted.js +++ b/test/parallel/test-http-client-spurious-aborted.js @@ -10,7 +10,7 @@ const N = 2; let abortRequest = true; const server = http.Server(common.mustCall((req, res) => { - const headers = { 'Content-Type': 'text/plain' }; + const headers = { 'Content-Type': 'text/plain', 'Connection': 'close' }; headers['Content-Length'] = 50; const socket = res.socket; res.writeHead(200, headers); diff --git a/test/parallel/test-http-client-timeout-on-connect.js b/test/parallel/test-http-client-timeout-on-connect.js index 3a0098229d9af8..47b9fa800be527 100644 --- a/test/parallel/test-http-client-timeout-on-connect.js +++ b/test/parallel/test-http-client-timeout-on-connect.js @@ -13,7 +13,10 @@ const server = http.createServer((req, res) => { server.listen(0, common.localhostIPv4, common.mustCall(() => { const port = server.address().port; - const req = http.get(`http://${common.localhostIPv4}:${port}`); + const req = http.get( + `http://${common.localhostIPv4}:${port}`, + { agent: new http.Agent() } + ); req.setTimeout(1); req.on('socket', common.mustCall((socket) => { diff --git a/test/parallel/test-http-content-length.js b/test/parallel/test-http-content-length.js index e6ba3719f95ba6..193ee5a8831e60 100644 --- a/test/parallel/test-http-content-length.js +++ b/test/parallel/test-http-content-length.js @@ -5,17 +5,17 @@ const http = require('http'); const Countdown = require('../common/countdown'); const expectedHeadersMultipleWrites = { - 'connection': 'close', + 'connection': 'keep-alive', 'transfer-encoding': 'chunked', }; const expectedHeadersEndWithData = { - 'connection': 'close', - 'content-length': String('hello world'.length) + 'connection': 'keep-alive', + 'content-length': String('hello world'.length), }; const expectedHeadersEndNoData = { - 'connection': 'close', + 'connection': 'keep-alive', 'content-length': '0', }; @@ -24,6 +24,7 @@ const countdown = new Countdown(3, () => server.close()); const server = http.createServer(function(req, res) { res.removeHeader('Date'); + res.setHeader('Keep-Alive', 'timeout=1'); switch (req.url.substr(1)) { case 'multiple-writes': @@ -59,7 +60,8 @@ server.listen(0, function() { req.write('hello '); req.end('world'); req.on('response', function(res) { - assert.deepStrictEqual(res.headers, expectedHeadersMultipleWrites); + assert.deepStrictEqual(res.headers, { ...expectedHeadersMultipleWrites, 'keep-alive': 'timeout=1' }); + res.resume(); }); req = http.request({ @@ -71,7 +73,8 @@ server.listen(0, function() { req.removeHeader('Host'); req.end('hello world'); req.on('response', function(res) { - assert.deepStrictEqual(res.headers, expectedHeadersEndWithData); + assert.deepStrictEqual(res.headers, { ...expectedHeadersEndWithData, 'keep-alive': 'timeout=1' }); + res.resume(); }); req = http.request({ @@ -83,7 +86,8 @@ server.listen(0, function() { req.removeHeader('Host'); req.end(); req.on('response', function(res) { - assert.deepStrictEqual(res.headers, expectedHeadersEndNoData); + assert.deepStrictEqual(res.headers, { ...expectedHeadersEndNoData, 'keep-alive': 'timeout=1' }); + res.resume(); }); }); diff --git a/test/parallel/test-http-default-encoding.js b/test/parallel/test-http-default-encoding.js index 8fcbf1a07f26ff..0c0de0808ae8c2 100644 --- a/test/parallel/test-http-default-encoding.js +++ b/test/parallel/test-http-default-encoding.js @@ -32,9 +32,9 @@ const server = http.Server((req, res) => { req.on('data', (chunk) => { result += chunk; }).on('end', () => { - server.close(); res.writeHead(200); res.end('hello world\n'); + server.close(); }); }); diff --git a/test/parallel/test-http-max-headers-count.js b/test/parallel/test-http-max-headers-count.js index b707c62bfba8c7..94685b228845aa 100644 --- a/test/parallel/test-http-max-headers-count.js +++ b/test/parallel/test-http-max-headers-count.js @@ -48,7 +48,7 @@ const server = http.createServer(function(req, res) { expected = maxAndExpected[requests][1]; server.maxHeadersCount = max; } - res.writeHead(200, headers); + res.writeHead(200, { ...headers, 'Connection': 'close' }); res.end(); }); server.maxHeadersCount = max; diff --git a/test/parallel/test-http-outgoing-message-capture-rejection.js b/test/parallel/test-http-outgoing-message-capture-rejection.js index a19176df415263..a89ef8baf70605 100644 --- a/test/parallel/test-http-outgoing-message-capture-rejection.js +++ b/test/parallel/test-http-outgoing-message-capture-rejection.js @@ -16,9 +16,11 @@ events.captureRejections = true; res.socket.on('error', common.mustCall((err) => { assert.strictEqual(err, _err); + server.close(); })); // Write until there is space in the buffer + res.writeHead(200, { 'Connection': 'close' }); while (res.write('hello')); })); @@ -37,7 +39,6 @@ events.captureRejections = true; code: 'ECONNRESET' })); res.resume(); - server.close(); })); })); } diff --git a/test/parallel/test-http-raw-headers.js b/test/parallel/test-http-raw-headers.js index d77a7cacc64c29..adc5bec1712201 100644 --- a/test/parallel/test-http-raw-headers.js +++ b/test/parallel/test-http-raw-headers.js @@ -34,13 +34,13 @@ http.createServer(function(req, res) { 'x-BaR', 'yoyoyo', 'Connection', - 'close', + 'keep-alive', ]; const expectHeaders = { 'host': `localhost:${this.address().port}`, 'transfer-encoding': 'CHUNKED', 'x-bar': 'yoyoyo', - 'connection': 'close' + 'connection': 'keep-alive' }; const expectRawTrailers = [ 'x-bAr', @@ -65,6 +65,7 @@ http.createServer(function(req, res) { }); req.resume(); + res.setHeader('Keep-Alive', 'timeout=1'); res.setHeader('Trailer', 'x-foo'); res.addTrailers([ ['x-fOo', 'xOxOxOx'], @@ -86,22 +87,25 @@ http.createServer(function(req, res) { req.end('y b a r'); req.on('response', function(res) { const expectRawHeaders = [ + 'Keep-Alive', + 'timeout=1', 'Trailer', 'x-foo', 'Date', null, 'Connection', - 'close', + 'keep-alive', 'Transfer-Encoding', 'chunked', ]; const expectHeaders = { + 'keep-alive': 'timeout=1', 'trailer': 'x-foo', 'date': null, - 'connection': 'close', + 'connection': 'keep-alive', 'transfer-encoding': 'chunked' }; - res.rawHeaders[3] = null; + res.rawHeaders[5] = null; res.headers.date = null; assert.deepStrictEqual(res.rawHeaders, expectRawHeaders); assert.deepStrictEqual(res.headers, expectHeaders); diff --git a/test/parallel/test-http-request-end.js b/test/parallel/test-http-request-end.js index c9278945b7993b..d762f459db27f0 100644 --- a/test/parallel/test-http-request-end.js +++ b/test/parallel/test-http-request-end.js @@ -36,9 +36,9 @@ const server = http.Server(function(req, res) { req.on('end', function() { assert.strictEqual(result, expected); - server.close(); res.writeHead(200); res.end('hello world\n'); + server.close(); }); }); diff --git a/test/parallel/test-http-should-keep-alive.js b/test/parallel/test-http-should-keep-alive.js index dfc99cf4f3b3a1..5cff29adbc54db 100644 --- a/test/parallel/test-http-should-keep-alive.js +++ b/test/parallel/test-http-should-keep-alive.js @@ -50,6 +50,10 @@ const getCountdownIndex = () => SERVER_RESPONSES.length - countdown.remaining; const server = net.createServer(function(socket) { socket.write(SERVER_RESPONSES[getCountdownIndex()]); + + if (SHOULD_KEEP_ALIVE[getCountdownIndex()]) { + socket.end(); + } }).listen(0, function() { function makeRequest() { const req = http.get({ port: server.address().port }, function(res) { diff --git a/test/parallel/test-http-unix-socket-keep-alive.js b/test/parallel/test-http-unix-socket-keep-alive.js index e1a2267e5c9689..36a3a4f535f07b 100644 --- a/test/parallel/test-http-unix-socket-keep-alive.js +++ b/test/parallel/test-http-unix-socket-keep-alive.js @@ -20,7 +20,7 @@ server.listen(common.PIPE, common.mustCall(() => function asyncLoop(fn, times, cb) { fn(function handler() { if (--times) { - fn(handler); + setTimeout(() => fn(handler), common.platformTimeout(10)); } else { cb(); } diff --git a/test/parallel/test-https-agent-session-eviction.js b/test/parallel/test-https-agent-session-eviction.js index 940c43cc40bf15..20cdb870a09ddb 100644 --- a/test/parallel/test-https-agent-session-eviction.js +++ b/test/parallel/test-https-agent-session-eviction.js @@ -19,6 +19,7 @@ const options = { // Create TLS1.2 server https.createServer(options, function(req, res) { + res.writeHead(200, { 'Connection': 'close' }); res.end('ohai'); }).listen(0, function() { first(this); @@ -44,6 +45,7 @@ function first(server) { function faultyServer(port) { options.secureProtocol = 'TLSv1_method'; https.createServer(options, function(req, res) { + res.writeHead(200, { 'Connection': 'close' }); res.end('hello faulty'); }).listen(port, function() { second(this); diff --git a/test/parallel/test-https-max-headers-count.js b/test/parallel/test-https-max-headers-count.js index f8e4e64bfb94dd..0fdd38c73ed6f1 100644 --- a/test/parallel/test-https-max-headers-count.js +++ b/test/parallel/test-https-max-headers-count.js @@ -37,7 +37,7 @@ const server = https.createServer(serverOptions, common.mustCall((req, res) => { expected = maxAndExpected[requests][1]; server.maxHeadersCount = max; } - res.writeHead(200, headers); + res.writeHead(200, { ...headers, 'Connection': 'close' }); res.end(); }, 3)); server.maxHeadersCount = max; diff --git a/test/parallel/test-stream-destroy.js b/test/parallel/test-stream-destroy.js index f99c29c811932d..77c3a0aaa3fea5 100644 --- a/test/parallel/test-stream-destroy.js +++ b/test/parallel/test-stream-destroy.js @@ -62,7 +62,8 @@ const http = require('http'); server.listen(0, () => { const req = http.request({ - port: server.address().port + port: server.address().port, + agent: new http.Agent() }); req.write('asd'); @@ -96,7 +97,8 @@ const http = require('http'); server.listen(0, () => { const req = http.request({ - port: server.address().port + port: server.address().port, + agent: new http.Agent() }); req.write('asd'); diff --git a/test/parallel/test-tls-over-http-tunnel.js b/test/parallel/test-tls-over-http-tunnel.js index b26cf7872f6582..503ec1f6600f5f 100644 --- a/test/parallel/test-tls-over-http-tunnel.js +++ b/test/parallel/test-tls-over-http-tunnel.js @@ -62,7 +62,7 @@ const proxy = net.createServer((clientSocket) => { 'HTTP/1.1\r\n' + 'Proxy-Connections: keep-alive\r\n' + `Host: localhost:${proxy.address().port}\r\n` + - 'Connection: close\r\n\r\n'); + 'Connection: keep-alive\r\n\r\n'); console.log('PROXY: got CONNECT request'); console.log('PROXY: creating a tunnel'); diff --git a/test/parallel/test-tls-set-secure-context.js b/test/parallel/test-tls-set-secure-context.js index d62b6b9f94f601..f41a94cbf229b8 100644 --- a/test/parallel/test-tls-set-secure-context.js +++ b/test/parallel/test-tls-set-secure-context.js @@ -82,7 +82,8 @@ function makeRequest(port, id) { rejectUnauthorized: true, ca: credentialOptions[0].ca, servername: 'agent1', - headers: { id } + headers: { id }, + agent: new https.Agent() }; let errored = false; diff --git a/test/sequential/test-http-econnrefused.js b/test/sequential/test-http-econnrefused.js index e1abde32bd81b1..7f8c2cee56c4c8 100644 --- a/test/sequential/test-http-econnrefused.js +++ b/test/sequential/test-http-econnrefused.js @@ -43,7 +43,7 @@ const server = http.createServer(function(req, res) { req.on('end', function() { assert.strictEqual(body, 'PING'); - res.writeHead(200); + res.writeHead(200, { 'Connection': 'close' }); res.end('PONG'); }); });