From 74b0e8c3a82f678c5b6bf5cca63b9d27c4d5963e 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 3db42174d73004..b9d2c0c2c04a7c 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 73a6409c411cc1..c78289138e756a 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 00000000000000..84ed5e57c5fb86
--- /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);
+  }));