From 65790af582998c9c3b820a2dee84da5634455343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Tue, 1 Nov 2016 16:59:31 +0100 Subject: [PATCH 01/10] http: remove stale timeout listeners In order to prevent a memory leak when using keep alive, ensure that the timeout listener for the request is removed when the response has ended. --- lib/_http_client.js | 8 ++- ...st-http-client-timeout-option-listeners.js | 56 +++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http-client-timeout-option-listeners.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 6837c94df98eea..5b581c3d8cf0e7 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -562,7 +562,13 @@ function tickOnSocket(req, socket) { socket.on('close', socketCloseListener); if (req.timeout) { - socket.once('timeout', () => req.emit('timeout')); + const emitRequestTimeout = () => req.emit('timeout'); + socket.once('timeout', emitRequestTimeout); + req.on('response', (r) => { + r.on('end', () => { + socket.removeListener('timeout', emitRequestTimeout); + }); + }); } req.emit('socket', socket); } diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js new file mode 100644 index 00000000000000..79366d27b2aecf --- /dev/null +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -0,0 +1,56 @@ +'use strict'; +require('../common'); +const http = require('http'); +const assert = require('assert'); + +const agent = new http.Agent({ keepAlive: true }); + +const server = http.createServer((req, res) => { + res.end(''); +}); + +const options = { + agent, + method: 'GET', + port: undefined, + host: '127.0.0.1', + path: '/', + timeout: 10 +}; + +server.listen(0, options.host, (e) => { + options.port = server.address().port; + + doRequest().then(doRequest) + .then((numListeners) => { + assert.strictEqual(numListeners, 1, + 'Should be a single timeout listener on the socket'); + server.close(); + agent.destroy(); + }) + .catch((e) => { + console.log(e); + process.exit(1); + }); +}); + +function doRequest() { + return new Promise((resolve, reject) => { + http.request(options, (response) => { + const sockets = agent.sockets[`127.0.0.1:${options.port}:`]; + if (sockets.length !== 1) { + reject(new Error('Only one socket should be created')); + } + const socket = sockets[0]; + const timeoutEvent = socket._events.timeout; + let numListeners = 0; + if (Array.isArray(timeoutEvent)) { + numListeners = timeoutEvent.length; + } else if (timeoutEvent) { + numListeners = 1; + } + response.resume(); + response.on('end', () => resolve(numListeners)); + }).end(); + }); +} From 5c37b4a2ea6782a572f3612901f6f89394aef7dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Thu, 3 Nov 2016 17:59:59 +0100 Subject: [PATCH 02/10] replace catch with unhandledRejection-listener --- .../test-http-client-timeout-option-listeners.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index 79366d27b2aecf..f6cbe709e99706 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -1,5 +1,5 @@ 'use strict'; -require('../common'); +const common = require('../common'); const http = require('http'); const assert = require('assert'); @@ -18,6 +18,10 @@ const options = { timeout: 10 }; +process.on('unhandledRejection', function() { + common.fail('A promise rejection was unhandled'); +}); + server.listen(0, options.host, (e) => { options.port = server.address().port; @@ -27,10 +31,6 @@ server.listen(0, options.host, (e) => { 'Should be a single timeout listener on the socket'); server.close(); agent.destroy(); - }) - .catch((e) => { - console.log(e); - process.exit(1); }); }); From 274cb28598dd2ca4ed523fd4a1d01dd0c275803c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Fri, 4 Nov 2016 09:16:39 +0100 Subject: [PATCH 03/10] use common.localhostIPv4 --- test/parallel/test-http-client-timeout-option-listeners.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index f6cbe709e99706..1d761d9aeb4626 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -13,7 +13,7 @@ const options = { agent, method: 'GET', port: undefined, - host: '127.0.0.1', + host: common.localhostIPv4, path: '/', timeout: 10 }; @@ -37,7 +37,7 @@ server.listen(0, options.host, (e) => { function doRequest() { return new Promise((resolve, reject) => { http.request(options, (response) => { - const sockets = agent.sockets[`127.0.0.1:${options.port}:`]; + const sockets = agent.sockets[`${options.host}:${options.port}:`]; if (sockets.length !== 1) { reject(new Error('Only one socket should be created')); } From c32ee77b2bc1012ea7ffcdcf3e871db9d4e21631 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Fri, 18 Nov 2016 16:59:05 +0100 Subject: [PATCH 04/10] Increase timeout, output rejection reason --- .../test-http-client-timeout-option-listeners.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index 1d761d9aeb4626..f396e94061a657 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -15,11 +15,11 @@ const options = { port: undefined, host: common.localhostIPv4, path: '/', - timeout: 10 + timeout: 100 }; -process.on('unhandledRejection', function() { - common.fail('A promise rejection was unhandled'); +process.on('unhandledRejection', function(reason) { + common.fail('A promise rejection was unhandled: ' + reason); }); server.listen(0, options.host, (e) => { @@ -27,8 +27,7 @@ server.listen(0, options.host, (e) => { doRequest().then(doRequest) .then((numListeners) => { - assert.strictEqual(numListeners, 1, - 'Should be a single timeout listener on the socket'); + assert.strictEqual(numListeners, 1); server.close(); agent.destroy(); }); From 2c4ebaee633f3499ea90cbe44ef230787791d15b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Fri, 18 Nov 2016 17:17:48 +0100 Subject: [PATCH 05/10] use common.platformTimeout --- test/parallel/test-http-client-timeout-option-listeners.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index f396e94061a657..c924fff6bd95f2 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -15,7 +15,7 @@ const options = { port: undefined, host: common.localhostIPv4, path: '/', - timeout: 100 + timeout: common.platformTimeout(20) }; process.on('unhandledRejection', function(reason) { From fa6ec18d49750e46cf700c0880cbb64bf758b310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Fri, 18 Nov 2016 17:45:11 +0100 Subject: [PATCH 06/10] early exit on rejection --- test/parallel/test-http-client-timeout-option-listeners.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index c924fff6bd95f2..fe3829f7dd953f 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -38,7 +38,7 @@ function doRequest() { http.request(options, (response) => { const sockets = agent.sockets[`${options.host}:${options.port}:`]; if (sockets.length !== 1) { - reject(new Error('Only one socket should be created')); + return reject(new Error('Only one socket should be created')); } const socket = sockets[0]; const timeoutEvent = socket._events.timeout; From 516ba6f0f746ca0420147599d2f173f94a145b4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Fri, 25 Nov 2016 16:40:13 +0100 Subject: [PATCH 07/10] use EventEmitter#once --- lib/_http_client.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 5b581c3d8cf0e7..1846e36e12bf88 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -564,8 +564,8 @@ function tickOnSocket(req, socket) { if (req.timeout) { const emitRequestTimeout = () => req.emit('timeout'); socket.once('timeout', emitRequestTimeout); - req.on('response', (r) => { - r.on('end', () => { + req.once('response', (res) => { + res.once('end', () => { socket.removeListener('timeout', emitRequestTimeout); }); }); From 60ff0fb12e5ba487ac09e27a6a97b1f03c3986d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Fri, 25 Nov 2016 17:52:14 +0100 Subject: [PATCH 08/10] test in callback-style --- ...st-http-client-timeout-option-listeners.js | 48 ++++++++----------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index fe3829f7dd953f..f74b701a0ceafe 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -15,41 +15,35 @@ const options = { port: undefined, host: common.localhostIPv4, path: '/', - timeout: common.platformTimeout(20) + timeout: common.platformTimeout(100) }; -process.on('unhandledRejection', function(reason) { - common.fail('A promise rejection was unhandled: ' + reason); -}); - server.listen(0, options.host, (e) => { options.port = server.address().port; - - doRequest().then(doRequest) - .then((numListeners) => { + doRequest(() => { + doRequest((numListeners) => { assert.strictEqual(numListeners, 1); server.close(); agent.destroy(); }); + }); }); -function doRequest() { - return new Promise((resolve, reject) => { - http.request(options, (response) => { - const sockets = agent.sockets[`${options.host}:${options.port}:`]; - if (sockets.length !== 1) { - return reject(new Error('Only one socket should be created')); - } - const socket = sockets[0]; - const timeoutEvent = socket._events.timeout; - let numListeners = 0; - if (Array.isArray(timeoutEvent)) { - numListeners = timeoutEvent.length; - } else if (timeoutEvent) { - numListeners = 1; - } - response.resume(); - response.on('end', () => resolve(numListeners)); - }).end(); - }); +function doRequest(cb) { + http.request(options, (response) => { + const sockets = agent.sockets[`${options.host}:${options.port}:`]; + assert.strictEqual(sockets.length, 1) + const socket = sockets[0]; + const timeoutEvent = socket._events.timeout; + let numListeners = 0; + if (Array.isArray(timeoutEvent)) { + numListeners = timeoutEvent.length; + } else if (timeoutEvent) { + numListeners = 1; + } + response.resume(); + response.once('end', () => { + process.nextTick(cb, numListeners); + }); + }).end(); } From 3039b1dd58ed300bf5ad6fff659881da68b3ac7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Sun, 27 Nov 2016 17:45:16 +0100 Subject: [PATCH 09/10] use socket.listeners, add common.mustCall --- ...st-http-client-timeout-option-listeners.js | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index f74b701a0ceafe..d8426af7de5a92 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -18,9 +18,10 @@ const options = { timeout: common.platformTimeout(100) }; -server.listen(0, options.host, (e) => { +server.listen(0, options.host, () => { options.port = server.address().port; - doRequest(() => { + doRequest((numListeners) => { + assert.strictEqual(numListeners, 1); doRequest((numListeners) => { assert.strictEqual(numListeners, 1); server.close(); @@ -30,20 +31,14 @@ server.listen(0, options.host, (e) => { }); function doRequest(cb) { - http.request(options, (response) => { + http.request(options, common.mustCall((response) => { const sockets = agent.sockets[`${options.host}:${options.port}:`]; - assert.strictEqual(sockets.length, 1) + assert.strictEqual(sockets.length, 1); const socket = sockets[0]; - const timeoutEvent = socket._events.timeout; - let numListeners = 0; - if (Array.isArray(timeoutEvent)) { - numListeners = timeoutEvent.length; - } else if (timeoutEvent) { - numListeners = 1; - } + const numListeners = socket.listeners('timeout').length; response.resume(); response.once('end', () => { process.nextTick(cb, numListeners); }); - }).end(); + })).end(); } From 6cc3f0b065fe08d77856adec99a7a102e3867275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20B=C3=B6hlmark?= Date: Mon, 28 Nov 2016 20:30:52 +0100 Subject: [PATCH 10/10] add common.mustCall --- .../test-http-client-timeout-option-listeners.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js index d8426af7de5a92..3ac6cd4616b496 100644 --- a/test/parallel/test-http-client-timeout-option-listeners.js +++ b/test/parallel/test-http-client-timeout-option-listeners.js @@ -18,17 +18,17 @@ const options = { timeout: common.platformTimeout(100) }; -server.listen(0, options.host, () => { +server.listen(0, options.host, common.mustCall(() => { options.port = server.address().port; - doRequest((numListeners) => { + doRequest(common.mustCall((numListeners) => { assert.strictEqual(numListeners, 1); - doRequest((numListeners) => { + doRequest(common.mustCall((numListeners) => { assert.strictEqual(numListeners, 1); server.close(); agent.destroy(); - }); - }); -}); + })); + })); +})); function doRequest(cb) { http.request(options, common.mustCall((response) => { @@ -37,8 +37,8 @@ function doRequest(cb) { const socket = sockets[0]; const numListeners = socket.listeners('timeout').length; response.resume(); - response.once('end', () => { + response.once('end', common.mustCall(() => { process.nextTick(cb, numListeners); - }); + })); })).end(); }