From 225ddd5f425313908cf53d5d29fe746612d31e4c 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 a0ee27c653e61b..40eb2a352b8e70 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -60,6 +60,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);
@@ -85,6 +92,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();
@@ -121,6 +133,7 @@ function Agent(options) {
             socket.setTimeout(agentTimeout);
           }
 
+          socket.once('error', freeSocketErrorListener);
           freeSockets.push(socket);
         } else {
           // Implementation doesn't want to keep socket alive
@@ -391,6 +404,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 e5b35bf0ca9162..dda29e3e8e6be5 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -441,13 +441,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;
@@ -622,8 +615,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
@@ -692,7 +688,6 @@ function tickOnSocket(req, socket) {
   }
 
   parser.onIncoming = parserOnIncomingClient;
-  socket.removeListener('error', freeSocketErrorListener);
   socket.on('error', socketErrorListener);
   socket.on('data', socketOnData);
   socket.on('end', socketOnEnd);
@@ -732,6 +727,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);
 };