net: improve network family autoselection handle handling

PR-URL: nodejs#48464
Fixes: npm/cli#6409
Fixes: KararTY/dank-twitch-irc#13
Fixes: nodejs#47644
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>

Author: ShogunPanda

lib/net.js

@@ -56,6 +56,7 @@ const {
   UV_EINVAL,
   UV_ENOTCONN,
   UV_ECANCELED,
+  UV_ETIMEDOUT,
 } = internalBinding('uv');
 
 const { Buffer } = require('buffer');
@@ -482,6 +483,10 @@ function Socket(options) {
     }
   }
 
+  if (options.signal) {
+    addClientAbortSignalOption(this, options);
+  }
+
   // Reserve properties
   this.server = null;
   this._server = null;
@@ -1091,6 +1096,11 @@ function internalConnectMultiple(context, canceled) {
   clearTimeout(context[kTimeout]);
   const self = context.socket;
 
+  // We were requested to abort. Stop all operations
+  if (self._aborted) {
+    return;
+  }
+
   // All connections have been tried without success, destroy with error
   if (canceled || context.current === context.addresses.length) {
     if (context.errors.length === 0) {
@@ -1105,7 +1115,11 @@ function internalConnectMultiple(context, canceled) {
   assert(self.connecting);
 
   const current = context.current++;
-  const handle = current === 0 ? self._handle : new TCP(TCPConstants.SOCKET);
+
+  if (current > 0) {
+    self[kReinitializeHandle](new TCP(TCPConstants.SOCKET));
+  }
+
   const { localPort, port, flags } = context;
   const { address, family: addressType } = context.addresses[current];
   let localAddress;
@@ -1114,16 +1128,16 @@ function internalConnectMultiple(context, canceled) {
   if (localPort) {
     if (addressType === 4) {
       localAddress = DEFAULT_IPV4_ADDR;
-      err = handle.bind(localAddress, localPort);
+      err = self._handle.bind(localAddress, localPort);
     } else { // addressType === 6
       localAddress = DEFAULT_IPV6_ADDR;
-      err = handle.bind6(localAddress, localPort, flags);
+      err = self._handle.bind6(localAddress, localPort, flags);
     }
 
     debug('connect/multiple: binding to localAddress: %s and localPort: %d (addressType: %d)',
           localAddress, localPort, addressType);
 
-    err = checkBindError(err, localPort, handle);
+    err = checkBindError(err, localPort, self._handle);
     if (err) {
       ArrayPrototypePush(context.errors, exceptionWithHostPort(err, 'bind', localAddress, localPort));
       internalConnectMultiple(context);
@@ -1143,9 +1157,9 @@ function internalConnectMultiple(context, canceled) {
   ArrayPrototypePush(self.autoSelectFamilyAttemptedAddresses, `${address}:${port}`);
 
   if (addressType === 4) {
-    err = handle.connect(req, address, port);
+    err = self._handle.connect(req, address, port);
   } else {
-    err = handle.connect6(req, address, port);
+    err = self._handle.connect6(req, address, port);
   }
 
   if (err) {
@@ -1165,7 +1179,7 @@ function internalConnectMultiple(context, canceled) {
     debug('connect/multiple: setting the attempt timeout to %d ms', context.timeout);
 
     // If the attempt has not returned an error, start the connection timer
-    context[kTimeout] = setTimeout(internalConnectMultipleTimeout, context.timeout, context, req, handle);
+    context[kTimeout] = setTimeout(internalConnectMultipleTimeout, context.timeout, context, req, self._handle);
   }
 }
 
@@ -1183,6 +1197,15 @@ Socket.prototype.connect = function(...args) {
   const options = normalized[0];
   const cb = normalized[1];
 
+  if (cb !== null) {
+    this.once('connect', cb);
+  }
+
+  // If the parent is already connecting, do not attempt to connect again
+  if (this._parent && this._parent.connecting) {
+    return this;
+  }
+
   // options.port === null will be checked later.
   if (options.port === undefined && options.path == null)
     throw new ERR_MISSING_ARGS(['options', 'port', 'path']);
@@ -1207,10 +1230,6 @@ Socket.prototype.connect = function(...args) {
     initSocketHandle(this);
   }
 
-  if (cb !== null) {
-    this.once('connect', cb);
-  }
-
   this._unrefTimer();
 
   this.connecting = true;
@@ -1583,7 +1602,47 @@ function afterConnect(status, handle, req, readable, writable) {
   }
 }
 
+function addClientAbortSignalOption(self, options) {
+  validateAbortSignal(options.signal, 'options.signal');
+  const { signal } = options;
+
+  function onAbort() {
+    signal.removeEventListener('abort', onAbort);
+    self._aborted = true;
+  }
+
+  if (signal.aborted) {
+    process.nextTick(onAbort);
+  } else {
+    process.nextTick(() => {
+      signal.addEventListener('abort', onAbort);
+    });
+  }
+}
+
+function createConnectionError(req, status) {
+  let details;
+
+  if (req.localAddress && req.localPort) {
+    details = req.localAddress + ':' + req.localPort;
+  }
+
+  const ex = exceptionWithHostPort(status,
+                                   'connect',
+                                   req.address,
+                                   req.port,
+                                   details);
+  if (details) {
+    ex.localAddress = req.localAddress;
+    ex.localPort = req.localPort;
+  }
+
+  return ex;
+}
+
 function afterConnectMultiple(context, current, status, handle, req, readable, writable) {
+  debug('connect/multiple: connection attempt to %s:%s completed with status %s', req.address, req.port, status);
+
   // Make sure another connection is not spawned
   clearTimeout(context[kTimeout]);
 
@@ -1596,35 +1655,15 @@ function afterConnectMultiple(context, current, status, handle, req, readable, w
 
   const self = context.socket;
 
-
   // Some error occurred, add to the list of exceptions
   if (status !== 0) {
-    let details;
-    if (req.localAddress && req.localPort) {
-      details = req.localAddress + ':' + req.localPort;
-    }
-    const ex = exceptionWithHostPort(status,
-                                     'connect',
-                                     req.address,
-                                     req.port,
-                                     details);
-    if (details) {
-      ex.localAddress = req.localAddress;
-      ex.localPort = req.localPort;
-    }
-
-    ArrayPrototypePush(context.errors, ex);
+    ArrayPrototypePush(context.errors, createConnectionError(req, status));
 
     // Try the next address
     internalConnectMultiple(context, status === UV_ECANCELED);
     return;
   }
 
-  if (context.current > 1 && self[kReinitializeHandle]) {
-    self[kReinitializeHandle](handle);
-    handle = self._handle;
-  }
-
   if (hasObserver('net')) {
     startPerf(
       self,
@@ -1633,17 +1672,18 @@ function afterConnectMultiple(context, current, status, handle, req, readable, w
     );
   }
 
-  afterConnect(status, handle, req, readable, writable);
+  afterConnect(status, self._handle, req, readable, writable);
 }
 
 function internalConnectMultipleTimeout(context, req, handle) {
   debug('connect/multiple: connection to %s:%s timed out', req.address, req.port);
   req.oncomplete = undefined;
+  ArrayPrototypePush(context.errors, createConnectionError(req, UV_ETIMEDOUT));
   handle.close();
   internalConnectMultiple(context);
 }
 
-function addAbortSignalOption(self, options) {
+function addServerAbortSignalOption(self, options) {
   if (options?.signal === undefined) {
     return;
   }
@@ -1932,7 +1972,7 @@ Server.prototype.listen = function(...args) {
     listenInCluster(this, null, -1, -1, backlogFromArgs);
     return this;
   }
-  addAbortSignalOption(this, options);
+  addServerAbortSignalOption(this, options);
   // (handle[, backlog][, cb]) where handle is an object with a fd
   if (typeof options.fd === 'number' && options.fd >= 0) {
     listenInCluster(this, null, null, null, backlogFromArgs, options.fd);

test/internet/test-https-autoselectfamily-slow-timeout.js

@@ -11,7 +11,7 @@ const { request } = require('https');
 
 request(
   `https://${addresses.INET_HOST}/en`,
-  // Purposely set this to false because we want all connection but the last to fail
+  // Purposely set this to a low value because we want all connection but the last to fail
   { autoSelectFamily: true, autoSelectFamilyAttemptTimeout: 10 },
   (res) => {
     assert.strictEqual(res.statusCode, 200);

test/internet/test-net-autoselectfamily-timeout-close.js

@@ -0,0 +1,27 @@
+'use strict';
+
+const common = require('../common');
+const { addresses } = require('../common/internet');
+
+const assert = require('assert');
+const { connect } = require('net');
+
+// Test that when all errors are returned when no connections succeeded and that the close event is emitted
+{
+  const connection = connect({
+    host: addresses.INET_HOST,
+    port: 10,
+    autoSelectFamily: true,
+    // Purposely set this to a low value because we want all non last connection to fail due to early timeout
+    autoSelectFamilyAttemptTimeout: 10,
+  });
+
+  connection.on('close', common.mustCall());
+  connection.on('ready', common.mustNotCall());
+
+  connection.on('error', common.mustCall((error) => {
+    assert.ok(connection.autoSelectFamilyAttemptedAddresses.length > 0);
+    assert.strictEqual(error.constructor.name, 'AggregateError');
+    assert.ok(error.errors.length > 0);
+  }));
+}

test/internet/test-tls-autoselectfamily-backing-socket.js

@@ -0,0 +1,22 @@
+'use strict';
+
+const common = require('../common');
+const { addresses: { INET_HOST } } = require('../common/internet');
+
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+
+const { Socket } = require('net');
+const { TLSSocket } = require('tls');
+
+// Test that TLS connecting works with autoSelectFamily when using a backing socket
+{
+  const socket = new Socket();
+  const secureSocket = new TLSSocket(socket);
+
+  secureSocket.on('connect', common.mustCall(() => secureSocket.end()));
+
+  socket.connect({ host: INET_HOST, port: 443, servername: INET_HOST });
+  secureSocket.connect({ host: INET_HOST, port: 443, servername: INET_HOST });
+}