http: use 'connect' event only if socket is connecting

Fixes a bug that prevented `ClientRequest.prototype.setTimeout()` from
working properly when the socket was reused for multiple requests.

Fixes: #16716
Refs: #8895
PR-URL: #16725
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

lib/_http_client.js

@@ -728,9 +728,13 @@ ClientRequest.prototype.setTimeout = function setTimeout(msecs, callback) {
   }
 
   this.once('socket', function(sock) {
-    sock.once('connect', function() {
+    if (sock.connecting) {
+      sock.once('connect', function() {
+        sock.setTimeout(msecs, emitTimeout);
+      });
+    } else {
       sock.setTimeout(msecs, emitTimeout);
-    });
+    }
   });
 
   return this;

test/parallel/test-http-client-timeout-connect-listener.js

@@ -0,0 +1,42 @@
+'use strict';
+const common = require('../common');
+
+// This test ensures that `ClientRequest.prototype.setTimeout()` does
+// not add a listener for the `'connect'` event to the socket if the
+// socket is already connected.
+
+const assert = require('assert');
+const http = require('http');
+
+// Maximum allowed value for timeouts.
+const timeout = 2 ** 31 - 1;
+
+const server = http.createServer((req, res) => {
+  res.end();
+});
+
+server.listen(0, common.mustCall(() => {
+  const agent = new http.Agent({ keepAlive: true, maxSockets: 1 });
+  const options = { port: server.address().port, agent: agent };
+
+  doRequest(options, common.mustCall(() => {
+    const req = doRequest(options, common.mustCall(() => {
+      agent.destroy();
+      server.close();
+    }));
+
+    req.on('socket', common.mustCall((socket) => {
+      assert.strictEqual(socket.listenerCount('connect'), 0);
+    }));
+  }));
+}));
+
+function doRequest(options, callback) {
+  const req = http.get(options, (res) => {
+    res.on('end', callback);
+    res.resume();
+  });
+
+  req.setTimeout(timeout);
+  return req;
+}