net: ensure net.connect calls Socket connect

It's important for people who monkey-patch `Socket.prototype.connect`
that it's called by `net.connect` since it's not possible to
monkey-patch `net.connect` directly (as the `connect` function is called
directly by other parts of `lib/net.js` instead of calling the
`exports.connect` function).

Among the actors who monkey-patch `Socket.prototype.connect` are most
APM vendors, the async-listener module and the
continuation-local-storage module.

Related:
- #12342
- #12852

PR-URL: #12861
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luca Maraschi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jan Krems <[email protected]>

Author: watson

lib/net.js

@@ -87,15 +87,14 @@ function connect() {
   // TODO(joyeecheung): use destructuring when V8 is fast enough
   var normalized = normalizeArgs(args);
   var options = normalized[0];
-  var cb = normalized[1];
   debug('createConnection', normalized);
   var socket = new Socket(options);
 
   if (options.timeout) {
     socket.setTimeout(options.timeout);
   }
 
-  return realConnect.call(socket, options, cb);
+  return Socket.prototype.connect.call(socket, normalized);
 }
 
 
@@ -915,18 +914,23 @@ function internalConnect(
 
 
 Socket.prototype.connect = function() {
-  var args = new Array(arguments.length);
-  for (var i = 0; i < arguments.length; i++)
-    args[i] = arguments[i];
-  // TODO(joyeecheung): use destructuring when V8 is fast enough
-  var normalized = normalizeArgs(args);
-  var options = normalized[0];
-  var cb = normalized[1];
-  return realConnect.call(this, options, cb);
-};
-
+  let normalized;
+  // If passed an array, it's treated as an array of arguments that have
+  // already been normalized (so we don't normalize more than once). This has
+  // been solved before in https://github.com/nodejs/node/pull/12342, but was
+  // reverted as it had unintended side effects.
+  if (arguments.length === 1 && Array.isArray(arguments[0])) {
+    normalized = arguments[0];
+  } else {
+    var args = new Array(arguments.length);
+    for (var i = 0; i < arguments.length; i++)
+      args[i] = arguments[i];
+    // TODO(joyeecheung): use destructuring when V8 is fast enough
+    normalized = normalizeArgs(args);
+  }
+  const options = normalized[0];
+  const cb = normalized[1];
 
-function realConnect(options, cb) {
   if (this.write !== Socket.prototype.write)
     this.write = Socket.prototype.write;
 
@@ -967,7 +971,7 @@ function realConnect(options, cb) {
     lookupAndConnect(this, options);
   }
   return this;
-}
+};
 
 
 function lookupAndConnect(self, options) {

test/parallel/test-net-connect-call-socket-connect.js

@@ -0,0 +1,39 @@
+'use strict';
+const common = require('../common');
+
+// This test checks that calling `net.connect` internally calls
+// `Socket.prototype.connect`.
+//
+// This is important for people who monkey-patch `Socket.prototype.connect`
+// since it's not possible to monkey-patch `net.connect` directly (as the core
+// `connect` function is called internally in Node instead of calling the
+// `exports.connect` function).
+//
+// Monkey-patching of `Socket.prototype.connect` is done by - among others -
+// most APM vendors, the async-listener module and the
+// continuation-local-storage module.
+//
+// Related:
+// - https://github.com/nodejs/node/pull/12342
+// - https://github.com/nodejs/node/pull/12852
+
+const net = require('net');
+const Socket = net.Socket;
+
+// Monkey patch Socket.prototype.connect to check that it's called.
+const orig = Socket.prototype.connect;
+Socket.prototype.connect = common.mustCall(function() {
+  return orig.apply(this, arguments);
+});
+
+const server = net.createServer();
+
+server.listen(common.mustCall(function() {
+  const port = server.address().port;
+  const client = net.connect({port}, common.mustCall(function() {
+    client.end();
+  }));
+  client.on('end', common.mustCall(function() {
+    server.close();
+  }));
+}));