From 09e38634fa2c836f477369eff073e26a83945b93 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 28 Feb 2020 22:27:39 +0100 Subject: [PATCH 1/3] http: fix socket re-use races Whether and when a socket is destroyed or not after a timeout is up to the user. This leaves an edge case where a socket that has emitted 'timeout' might be re-used from the free pool. Even if destroy is called on the socket, it won't be removed from the freelist until 'close' which can happen several ticks later. Sockets are removed from the free list on the 'close' event. However, there is a delay between calling destroy() and 'close' being emitted. This means that it possible for a socket that has been destroyed to be re-used from the free list, causing unexpected failures. --- lib/_http_agent.js | 50 ++++++---- .../test-http-agent-timeout-option.js | 4 +- test/parallel/test-http-agent-timeout.js | 96 +++++++++++++++++++ .../test-http-client-set-timeout-after-end.js | 2 +- test/parallel/test-http-client-set-timeout.js | 2 +- ...st-http-client-timeout-option-listeners.js | 4 +- ...t-http-client-timeout-option-with-agent.js | 4 +- 7 files changed, 138 insertions(+), 24 deletions(-) create mode 100644 test/parallel/test-http-agent-timeout.js diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 5e3298b594d949..a527e84865c551 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -120,6 +120,12 @@ function Agent(options) { socket[async_id_symbol] = -1; socket._httpMessage = null; this.removeSocket(socket, options); + + const agentTimeout = this.options.timeout || 0; + if (socket.timeout !== agentTimeout) { + socket.setTimeout(agentTimeout); + } + freeSockets.push(socket); } else { // Implementation doesn't want to keep socket alive @@ -202,12 +208,21 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */, this.sockets[name] = []; } - const freeLen = this.freeSockets[name] ? this.freeSockets[name].length : 0; + const freeSockets = this.freeSockets[name]; + let socket; + if (freeSockets) { + while (freeSockets.length && freeSockets[0].destroyed) { + freeSockets.shift(); + } + socket = freeSockets.shift(); + if (!freeSockets.length) + delete this.freeSockets[name]; + } + + const freeLen = freeSockets ? freeSockets.length : 0; const sockLen = freeLen + this.sockets[name].length; - if (freeLen) { - // We have a free socket, so use that. - const socket = this.freeSockets[name].shift(); + if (socket) { // Guard against an uninitialized or user supplied Socket. const handle = socket._handle; if (handle && typeof handle.asyncReset === 'function') { @@ -216,10 +231,6 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */, socket[async_id_symbol] = handle.getAsyncId(); } - // don't leak - if (!this.freeSockets[name].length) - delete this.freeSockets[name]; - this.reuseSocket(socket, req); setRequestSocket(this, req, socket); this.sockets[name].push(socket); @@ -319,6 +330,20 @@ function installListeners(agent, s, options) { } s.on('close', onClose); + function onTimeout() { + debug('CLIENT socket onTimeout'); + + // Destroy if in free list. + // TODO(ronag): Always destroy, even if not in free list. + const sockets = agent.freeSockets; + for (const name of ObjectKeys(sockets)) { + if (sockets[name].includes(s)) { + return s.destroy(); + } + } + } + s.on('timeout', onTimeout); + function onRemove() { // We need this function for cases like HTTP 'upgrade' // (defined by WebSockets) where we need to remove a socket from the @@ -327,6 +352,7 @@ function installListeners(agent, s, options) { agent.removeSocket(s, options); s.removeListener('close', onClose); s.removeListener('free', onFree); + s.removeListener('timeout', onTimeout); s.removeListener('agentRemove', onRemove); } s.on('agentRemove', onRemove); @@ -409,14 +435,6 @@ function setRequestSocket(agent, req, socket) { return; } socket.setTimeout(req.timeout); - // Reset timeout after response end - req.once('response', (res) => { - res.once('end', () => { - if (socket.timeout !== agentTimeout) { - socket.setTimeout(agentTimeout); - } - }); - }); } function emitErrorNT(emitter, err) { diff --git a/test/parallel/test-http-agent-timeout-option.js b/test/parallel/test-http-agent-timeout-option.js index d0c05827f23d56..60a86779838520 100644 --- a/test/parallel/test-http-agent-timeout-option.js +++ b/test/parallel/test-http-agent-timeout-option.js @@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => { const listeners = socket.listeners('timeout'); - strictEqual(listeners.length, 1); - strictEqual(listeners[0], request.timeoutCb); + strictEqual(listeners.length, 2); + strictEqual(listeners[1], request.timeoutCb); })); diff --git a/test/parallel/test-http-agent-timeout.js b/test/parallel/test-http-agent-timeout.js new file mode 100644 index 00000000000000..f58a8965b4f123 --- /dev/null +++ b/test/parallel/test-http-agent-timeout.js @@ -0,0 +1,96 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +{ + // Ensure reuse of successful sockets. + + const agent = new http.Agent({ keepAlive: true }); + + const server = http.createServer((req, res) => { + res.end(); + }); + + server.listen(0, common.mustCall(() => { + let socket; + http.get({ port: server.address().port, agent }) + .on('response', common.mustCall((res) => { + socket = res.socket; + assert(socket); + res.resume(); + socket.on('free', common.mustCall(() => { + http.get({ port: server.address().port, agent }) + .on('response', common.mustCall((res) => { + assert.strictEqual(socket, res.socket); + assert(socket); + agent.destroy(); + server.close(); + })); + })); + })); + })); +} + +{ + // Ensure that timeouted sockets are not reused. + + const agent = new http.Agent({ keepAlive: true, timeout: 50 }); + + const server = http.createServer((req, res) => { + res.end(); + }); + + server.listen(0, common.mustCall(() => { + let socket; + http.get({ port: server.address().port, agent }) + .on('response', common.mustCall((res) => { + socket = res.socket; + assert(socket); + res.resume(); + socket.on('free', common.mustCall(() => { + socket.on('timeout', common.mustCall(() => { + http.get({ port: server.address().port, agent }) + .on('response', common.mustCall((res) => { + assert.notStrictEqual(socket, res.socket); + assert.strictEqual(socket.destroyed, true); + assert(socket); + agent.destroy(); + server.close(); + })); + })); + })); + })); + })); +} + +{ + // Ensure that destroyed sockets are not reused. + + const agent = new http.Agent({ keepAlive: true }); + + const server = http.createServer((req, res) => { + res.end(); + }); + + server.listen(0, common.mustCall(() => { + let socket; + http.get({ port: server.address().port, agent }) + .on('response', common.mustCall((res) => { + socket = res.socket; + assert(socket); + res.resume(); + socket.on('free', common.mustCall(() => { + socket.destroy(); + http.get({ port: server.address().port, agent }) + .on('response', common.mustCall((res) => { + assert.notStrictEqual(socket, res.socket); + assert(socket); + agent.destroy(); + server.close(); + })); + })); + })); + })); +} diff --git a/test/parallel/test-http-client-set-timeout-after-end.js b/test/parallel/test-http-client-set-timeout-after-end.js index 99bbf3dd1bc766..93eab80938a74b 100644 --- a/test/parallel/test-http-client-set-timeout-after-end.js +++ b/test/parallel/test-http-client-set-timeout-after-end.js @@ -20,7 +20,7 @@ server.listen(0, () => { const req = get({ agent, port }, (res) => { res.on('end', () => { strictEqual(req.setTimeout(0), req); - strictEqual(socket.listenerCount('timeout'), 0); + strictEqual(socket.listenerCount('timeout'), 1); agent.destroy(); server.close(); }); diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js index 7717b7d6069c73..51b6622a6b71cc 100644 --- a/test/parallel/test-http-client-set-timeout.js +++ b/test/parallel/test-http-client-set-timeout.js @@ -42,7 +42,7 @@ server.listen(0, mustCall(() => { })); req.on('timeout', mustCall(() => { - strictEqual(req.socket.listenerCount('timeout'), 0); + strictEqual(req.socket.listenerCount('timeout'), 1); req.destroy(); })); })); diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index 727b5fddf09624..dac89b5fd1a2bb 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -24,9 +24,9 @@ const options = { server.listen(0, options.host, common.mustCall(() => { options.port = server.address().port; doRequest(common.mustCall((numListeners) => { - assert.strictEqual(numListeners, 1); + assert.strictEqual(numListeners, 2); doRequest(common.mustCall((numListeners) => { - assert.strictEqual(numListeners, 1); + assert.strictEqual(numListeners, 2); server.close(); agent.destroy(); })); diff --git a/test/parallel/test-http-client-timeout-option-with-agent.js b/test/parallel/test-http-client-timeout-option-with-agent.js index 594dd1215f43e5..833c21c8929b72 100644 --- a/test/parallel/test-http-client-timeout-option-with-agent.js +++ b/test/parallel/test-http-client-timeout-option-with-agent.js @@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => { const listeners = socket.listeners('timeout'); - strictEqual(listeners.length, 1); - strictEqual(listeners[0], request.timeoutCb); + strictEqual(listeners.length, 2); + strictEqual(listeners[1], request.timeoutCb); })); From ff470e17d9ca15548b1ae82dc154855af454ba1f Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 29 Feb 2020 09:37:25 +0100 Subject: [PATCH 2/3] fixup: add doc note --- doc/api/http.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/api/http.md b/doc/api/http.md index cf1e5d76bf42bc..7d4269df6a0a1e 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -239,6 +239,9 @@ added: v0.11.4 An object which contains arrays of sockets currently awaiting use by the agent when `keepAlive` is enabled. Do not modify. +Sockets in the `freeSockets` list will be automatically destroyed and +removed from the array on `'timeout'`. + ### `agent.getName(options)`