From ff3615d5d9e9193ad2eacb0a5f912c31ddc0acf8 Mon Sep 17 00:00:00 2001 From: Robert Nagy <ronagy@icloud.com> Date: Fri, 28 Feb 2020 12:29:45 +0100 Subject: [PATCH] http: move free socket error handling to agent The http client should not know anything about free sockets. Let the agent handle its pool of sockets. PR-URL: https://github.com/nodejs/node/pull/32003 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> --- lib/_http_agent.js | 14 ++++++++++++++ lib/_http_client.js | 17 +++++++---------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/_http_agent.js b/lib/_http_agent.js index dc46c0e0bc7147..66e8632c9d1285 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -57,6 +57,13 @@ class ReusedHandle { } } +function freeSocketErrorListener(err) { + const socket = this; + debug('SOCKET ERROR on FREE socket:', err.message, err.stack); + socket.destroy(); + socket.emit('agentRemove'); +} + function Agent(options) { if (!(this instanceof Agent)) return new Agent(options); @@ -82,6 +89,11 @@ function Agent(options) { const name = this.getName(options); debug('agent.on(free)', name); + // TODO(ronag): socket.destroy(err) might have been called + // before coming here and have an 'error' scheduled. In the + // case of socket.destroy() below this 'error' has no handler + // and could cause unhandled exception. + if (socket.writable && this.requests[name] && this.requests[name].length) { const req = this.requests[name].shift(); @@ -118,6 +130,7 @@ function Agent(options) { socket.setTimeout(agentTimeout); } + socket.once('error', freeSocketErrorListener); freeSockets.push(socket); } else { // Implementation doesn't want to keep socket alive @@ -393,6 +406,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { Agent.prototype.reuseSocket = function reuseSocket(socket, req) { debug('have free socket'); + socket.removeListener('error', freeSocketErrorListener); req.reusedSocket = true; socket.ref(); }; diff --git a/lib/_http_client.js b/lib/_http_client.js index b1535815050db3..b414cd41a02041 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -444,13 +444,6 @@ function socketErrorListener(err) { socket.destroy(); } -function freeSocketErrorListener(err) { - const socket = this; - debug('SOCKET ERROR on FREE socket:', err.message, err.stack); - socket.destroy(); - socket.emit('agentRemove'); -} - function socketOnEnd() { const socket = this; const req = this._httpMessage; @@ -629,8 +622,11 @@ function responseKeepAlive(req) { socket.removeListener('error', socketErrorListener); socket.removeListener('data', socketOnData); socket.removeListener('end', socketOnEnd); - socket.once('error', freeSocketErrorListener); - // There are cases where _handle === null. Avoid those. Passing null to + + // TODO(ronag): Between here and emitFreeNT the socket + // has no 'error' handler. + + // There are cases where _handle === null. Avoid those. Passing undefined to // nextTick() will call getDefaultTriggerAsyncId() to retrieve the id. const asyncId = socket._handle ? socket._handle.getAsyncId() : undefined; // Mark this socket as available, AFTER user-added end @@ -699,7 +695,6 @@ function tickOnSocket(req, socket) { } parser.onIncoming = parserOnIncomingClient; - socket.removeListener('error', freeSocketErrorListener); socket.on('error', socketErrorListener); socket.on('data', socketOnData); socket.on('end', socketOnEnd); @@ -739,6 +734,8 @@ function listenSocketTimeout(req) { } ClientRequest.prototype.onSocket = function onSocket(socket) { + // TODO(ronag): Between here and onSocketNT the socket + // has no 'error' handler. process.nextTick(onSocketNT, this, socket); };