From 212a7a609d605c13294034f2f403632b626edf4c Mon Sep 17 00:00:00 2001
From: Thomas Watson <w@tson.dk>
Date: Fri, 5 May 2017 16:02:14 +0200
Subject: [PATCH] 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:
- https://github.com/nodejs/node/pull/12342
- https://github.com/nodejs/node/pull/12852

PR-URL: https://github.com/nodejs/node/pull/12861
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
---
 lib/net.js                                    | 32 ++++++++-------
 .../test-net-connect-call-socket-connect.js   | 39 +++++++++++++++++++
 2 files changed, 57 insertions(+), 14 deletions(-)
 create mode 100644 test/parallel/test-net-connect-call-socket-connect.js

diff --git a/lib/net.js b/lib/net.js
index d50b3729d9..f1061922ca 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -87,7 +87,6 @@ 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);
 
@@ -95,7 +94,7 @@ function connect() {
     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) {
diff --git a/test/parallel/test-net-connect-call-socket-connect.js b/test/parallel/test-net-connect-call-socket-connect.js
new file mode 100644
index 0000000000..8d3413d9aa
--- /dev/null
+++ b/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();
+  }));
+}));