http: ensure client request emits close

If socket creation failed then an error would be
emitted on the client request object, but not
'close' nor would destroyed be set to true.

PR-URL: https://github.com/nodejs/node/pull/33178
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lib/_http_agent.js

@@ -242,7 +242,12 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
   } else if (sockLen < this.maxSockets) {
     debug('call onSocket', sockLen, freeLen);
     // If we are under maxSockets create a new one.
-    this.createSocket(req, options, handleSocketCreation(this, req, true));
+    this.createSocket(req, options, (err, socket) => {
+      if (err)
+        req.onSocket(socket, err);
+      else
+        setRequestSocket(this, req, socket);
+    });
   } else {
     debug('wait for socket');
     // We are over limit so we'll add it to the queue.
@@ -388,8 +393,12 @@ Agent.prototype.removeSocket = function removeSocket(s, options) {
     debug('removeSocket, have a request, make a socket');
     const req = this.requests[name][0];
     // If we have pending requests and a socket gets closed make a new one
-    const socketCreationHandler = handleSocketCreation(this, req, false);
-    this.createSocket(req, options, socketCreationHandler);
+    this.createSocket(req, options, (err, socket) => {
+      if (err)
+        req.onSocket(socket, err);
+      else
+        socket.emit('free');
+    });
   }
 };
 
@@ -422,19 +431,6 @@ Agent.prototype.destroy = function destroy() {
   }
 };
 
-function handleSocketCreation(agent, request, informRequest) {
-  return function handleSocketCreation_Inner(err, socket) {
-    if (err) {
-      process.nextTick(emitErrorNT, request, err);
-      return;
-    }
-    if (informRequest)
-      setRequestSocket(agent, request, socket);
-    else
-      socket.emit('free');
-  };
-}
-
 function setRequestSocket(agent, req, socket) {
   req.onSocket(socket);
   const agentTimeout = agent.options.timeout || 0;
@@ -444,10 +440,6 @@ function setRequestSocket(agent, req, socket) {
   socket.setTimeout(req.timeout);
 }
 
-function emitErrorNT(emitter, err) {
-  emitter.emit('error', err);
-}
-
 module.exports = {
   Agent,
   globalAgent: new Agent()

lib/_http_client.js

@@ -371,10 +371,12 @@ function _destroy(req, socket, err) {
   // TODO (ronag): Check if socket was used at all (e.g. headersSent) and
   // re-use it in that case. `req.socket` just checks whether the socket was
   // assigned to the request and *might* have been used.
-  if (!req.agent || req.socket) {
+  if (socket && (!req.agent || req.socket)) {
     socket.destroy(err);
   } else {
-    socket.emit('free');
+    if (socket) {
+      socket.emit('free');
+    }
     if (!req.aborted && !err) {
       err = connResetException('socket hang up');
     }
@@ -776,15 +778,18 @@ function listenSocketTimeout(req) {
   }
 }
 
-ClientRequest.prototype.onSocket = function onSocket(socket) {
+ClientRequest.prototype.onSocket = function onSocket(socket, err) {
   // TODO(ronag): Between here and onSocketNT the socket
   // has no 'error' handler.
-  process.nextTick(onSocketNT, this, socket);
+  process.nextTick(onSocketNT, this, socket, err);
 };
 
-function onSocketNT(req, socket) {
+function onSocketNT(req, socket, err) {
   if (req.destroyed) {
     _destroy(req, socket, req[kError]);
+  } else if (err) {
+    req.destroyed = true;
+    _destroy(req, null, err);
   } else {
     tickOnSocket(req, socket);
   }

test/parallel/test-http-agent-close.js

@@ -0,0 +1,21 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const agent = new http.Agent();
+const _err = new Error('kaboom');
+agent.createSocket = function(req, options, cb) {
+  cb(_err);
+};
+
+const req = http
+  .request({
+    agent
+  })
+  .on('error', common.mustCall((err) => {
+    assert.strictEqual(err, _err);
+  }))
+  .on('close', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, true);
+  }));