From 027e1c706d25be75a790eb744835a0ebdc65452f Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Thu, 30 Apr 2020 23:36:19 +0200
Subject: [PATCH] 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                     | 32 ++++++++++----------------
 lib/_http_client.js                    | 15 ++++++++----
 test/parallel/test-http-agent-close.js | 21 +++++++++++++++++
 3 files changed, 43 insertions(+), 25 deletions(-)
 create mode 100644 test/parallel/test-http-agent-close.js

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index 3db42174d7..b9d2c0c2c0 100644
--- a/lib/_http_agent.js
+++ b/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()
diff --git a/lib/_http_client.js b/lib/_http_client.js
index 73a6409c41..c78289138e 100644
--- a/lib/_http_client.js
+++ b/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);
   }
diff --git a/test/parallel/test-http-agent-close.js b/test/parallel/test-http-agent-close.js
new file mode 100644
index 0000000000..84ed5e57c5
--- /dev/null
+++ b/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);
+  }));