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 · Apr 22, 2020 · 225ddd5 · 225ddd5
1 parent a40e7da
commit 225ddd5
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
@@ -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
@@ -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);
 };