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: #32003 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
nodejs · Mar 30, 2020 · ff3615d · ff3615d
1 parent 73fec7c
commit ff3615d
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 10 deletions.
diff --git 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
@@ -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);
 };