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: https://github.com/nodejs/node/issues/16716
Refs: https://github.com/nodejs/node/pull/8895
PR-URL: https://github.com/nodejs/node/pull/16725
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

1 files changed, 42 insertions, 0 deletions

diff --git a/test/parallel/test-http-client-timeout-connect-listener.js b/test/parallel/test-http-client-timeout-connect-listener.js
new file mode 100644
index 0000000000..ea09aff718
--- /dev/null
+++ b/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;
+}