From 9db1a1cdd806a7e5e4c8ad764a6044b029e1b0b0 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 14 Jul 2019 21:45:04 +0200 Subject: [PATCH 1/3] http: emit close on socket re-use --- lib/_http_client.js | 20 +++++++++++--- ...est-http-client-agent-abort-close-event.js | 23 ++++++++++++++++ .../test-http-client-agent-end-close-event.js | 27 +++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-http-client-agent-abort-close-event.js create mode 100644 test/parallel/test-http-client-agent-end-close-event.js diff --git a/lib/_http_client.js b/lib/_http_client.js index c7c27f9ad598a5..fd4f4887f2ebf5 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -594,6 +594,11 @@ function parserOnIncomingClient(res, shouldKeepAlive) { function responseKeepAlive(req) { const socket = req.socket; + req.emit('close'); + if (res) { + res.emit('close'); + } + debug('AGENT socket keep-alive'); if (req.timeoutCb) { socket.setTimeout(0, req.timeoutCb); @@ -635,15 +640,23 @@ function responseOnEnd() { // - when we have a socket we write directly to it without buffering. // - `req.finished` means `end()` has been called and no further data. // can be written - responseKeepAlive(req); + + // Run in next tick so any pending handlers + // have a chance to run first and perform any + // modifications. + process.nextTick(responseKeepAlive, res, req); } } function requestOnPrefinish() { const req = this; - if (req.shouldKeepAlive && req._ended) - responseKeepAlive(req); + if (req.shouldKeepAlive && req._ended) { + // Run in next tick so any pending handlers + // have a chance to run first and perform any + // modifications. + process.nextTick(responseKeepAlive, res, req); + } } function emitFreeNT(socket) { @@ -717,6 +730,7 @@ function onSocketNT(req, socket) { if (!req.agent) { socket.destroy(); } else { + req.emit('close'); socket.emit('free'); } } else { diff --git a/test/parallel/test-http-client-agent-abort-close-event.js b/test/parallel/test-http-client-agent-abort-close-event.js new file mode 100644 index 00000000000000..437d2aad222780 --- /dev/null +++ b/test/parallel/test-http-client-agent-abort-close-event.js @@ -0,0 +1,23 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); + +const server = http.createServer(common.mustNotCall()); + +const keepAliveAgent = new http.Agent({ keepAlive: true }); + +server.listen(0, common.mustCall(() => { + const req = http.get({ + port: server.address().port, + agent: keepAliveAgent + }); + + req + .on('socket', common.mustNotCall()) + .on('response', common.mustNotCall()) + .on('close', common.mustCall(() => { + server.close(); + keepAliveAgent.destroy(); + })) + .abort(); +})); diff --git a/test/parallel/test-http-client-agent-end-close-event.js b/test/parallel/test-http-client-agent-end-close-event.js new file mode 100644 index 00000000000000..de3ce193fbba9d --- /dev/null +++ b/test/parallel/test-http-client-agent-end-close-event.js @@ -0,0 +1,27 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); + +const server = http.createServer(common.mustCall((req, res) => { + res.end('hello'); +})); + +const keepAliveAgent = new http.Agent({ keepAlive: true }); + +server.listen(0, common.mustCall(() => { + const req = http.get({ + port: server.address().port, + agent: keepAliveAgent + }); + + req + .on('response', common.mustCall((res) => { + res + .on('close', common.mustCall(() => { + server.close(); + keepAliveAgent.destroy(); + })) + .on('data', common.mustCall()); + })) + .end(); +})); From efa5cbb541882d7a6d1fbdd9f259d21cd886dfd2 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 23 Sep 2019 18:40:27 +0200 Subject: [PATCH 2/3] fixup: typo from merge --- lib/_http_client.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index fd4f4887f2ebf5..b7c1e5fb35027f 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -593,6 +593,7 @@ function parserOnIncomingClient(res, shouldKeepAlive) { // client function responseKeepAlive(req) { const socket = req.socket; + const res = req.res; req.emit('close'); if (res) { @@ -644,7 +645,7 @@ function responseOnEnd() { // Run in next tick so any pending handlers // have a chance to run first and perform any // modifications. - process.nextTick(responseKeepAlive, res, req); + process.nextTick(responseKeepAlive, req); } } @@ -655,7 +656,7 @@ function requestOnPrefinish() { // Run in next tick so any pending handlers // have a chance to run first and perform any // modifications. - process.nextTick(responseKeepAlive, res, req); + process.nextTick(responseKeepAlive, req); } } From 55d1d3de7c576bff449ef213c521749800fda606 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 24 Sep 2019 00:44:21 +0200 Subject: [PATCH 3/3] fixup: pass test --- lib/_http_client.js | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index b7c1e5fb35027f..47e966f300000b 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -593,12 +593,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) { // client function responseKeepAlive(req) { const socket = req.socket; - const res = req.res; - - req.emit('close'); - if (res) { - res.emit('close'); - } debug('AGENT socket keep-alive'); if (req.timeoutCb) { @@ -613,7 +607,7 @@ function responseKeepAlive(req) { const asyncId = socket._handle ? socket._handle.getAsyncId() : undefined; // Mark this socket as available, AFTER user-added end // handlers have a chance to run. - defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, socket); + defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, req); } function responseOnEnd() { @@ -635,33 +629,32 @@ function responseOnEnd() { socket.end(); } assert(!socket.writable); - } else if (req.finished) { + } else if (req.finished && !this.aborted) { // We can assume `req.finished` means all data has been written since: // - `'responseOnEnd'` means we have been assigned a socket. // - when we have a socket we write directly to it without buffering. // - `req.finished` means `end()` has been called and no further data. // can be written - - // Run in next tick so any pending handlers - // have a chance to run first and perform any - // modifications. - process.nextTick(responseKeepAlive, req); + responseKeepAlive(req); } } function requestOnPrefinish() { const req = this; - if (req.shouldKeepAlive && req._ended) { - // Run in next tick so any pending handlers - // have a chance to run first and perform any - // modifications. - process.nextTick(responseKeepAlive, req); - } + if (req.shouldKeepAlive && req._ended) + responseKeepAlive(req); } -function emitFreeNT(socket) { - socket.emit('free'); +function emitFreeNT(req) { + req.emit('close'); + if (req.res) { + req.res.emit('close'); + } + + if (req.socket) { + req.socket.emit('free'); + } } function tickOnSocket(req, socket) {