net: enable autoSelectFamily by default

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #46790
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Author: 2 people

doc/api/cli.md

@@ -462,14 +462,20 @@ added: v6.0.0
 Enable FIPS-compliant crypto at startup. (Requires Node.js to be built
 against FIPS-compatible OpenSSL.)
 
-### `--enable-network-family-autoselection`
+### `--no-network-family-autoselection`
 
 <!-- YAML
 added: v19.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/46790
+    description: The flag was renamed from `--no-enable-network-family-autoselection`
+                 to `--no-network-family-autoselection`. The old name can still work as
+                 an alias.
 -->
 
-Enables the family autoselection algorithm unless connection options explicitly
-disables it.
+Disables the family autoselection algorithm unless connection options explicitly
+enables it.
 
 ### `--enable-source-maps`
 
@@ -2125,6 +2131,7 @@ Node.js options that are allowed are:
 * `--no-extra-info-on-fatal-exception`
 * `--no-force-async-hooks-checks`
 * `--no-global-search-paths`
+* `--no-network-family-autoselection`
 * `--no-warnings`
 * `--node-memory-debug`
 * `--openssl-config`

doc/api/errors.md

@@ -2593,6 +2593,13 @@ An attempt was made to operate on an already closed socket.
 When calling [`net.Socket.write()`][] on a connecting socket and the socket was
 closed before the connection was established.
 
+<a id="ERR_SOCKET_CONNECTION_TIMEOUT"></a>
+
+### `ERR_SOCKET_CONNECTION_TIMEOUT`
+
+The socket was unable to connect to any address returned by the DNS within the
+allowed timeout when using the family autoselection algorithm.
+
 <a id="ERR_SOCKET_DGRAM_IS_CONNECTED"></a>
 
 ### `ERR_SOCKET_DGRAM_IS_CONNECTED`

doc/api/net.md

@@ -870,6 +870,12 @@ behavior.
 <!-- YAML
 added: v0.1.90
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/46790
+    description: The default value for the autoSelectFamily option is now true.
+                 The `--enable-network-family-autoselection` CLI flag has been renamed
+                 to `--network-family-autoselection`. The old name is now an
+                 alias but it is discouraged.
   - version: v19.4.0
     pr-url: https://github.com/nodejs/node/pull/45777
     description: The default value for autoSelectFamily option can be changed
@@ -936,12 +942,12 @@ For TCP connections, available `options` are:
   option before timing out and trying the next address.
   Ignored if the `family` option is not `0` or if `localAddress` is set.
   Connection errors are not emitted if at least one connection succeeds.
-  **Default:** initially `false`, but it can be changed at runtime using [`net.setDefaultAutoSelectFamily(value)`][]
-  or via the command line option `--enable-network-family-autoselection`.
+  If all connections attempts fails, a single `AggregateError` with all failed attempts is emitted.
+  **Default:** [`net.getDefaultAutoSelectFamily()`][]
 * `autoSelectFamilyAttemptTimeout` {number}: The amount of time in milliseconds to wait
   for a connection attempt to finish before trying the next address when using the `autoSelectFamily` option.
   If set to a positive integer less than `10`, then the value `10` will be used instead.
-  **Default:** initially `250`, but it can be changed at runtime using [`net.setDefaultAutoSelectFamilyAttemptTimeout(value)`][]
+  **Default:** [`net.getDefaultAutoSelectFamilyAttemptTimeout()`][]
 
 For [IPC][] connections, available `options` are:
 
@@ -1629,6 +1635,8 @@ added: v19.4.0
 -->
 
 Gets the current default value of the `autoSelectFamily` option of [`socket.connect(options)`][].
+The initial default value is `true`, unless the command line option
+`--no-network-family-autoselection` is provided.
 
 * Returns: {boolean} The current default value of the `autoSelectFamily` option.
 
@@ -1649,6 +1657,7 @@ added: v19.8.0
 -->
 
 Gets the current default value of the `autoSelectFamilyAttemptTimeout` option of [`socket.connect(options)`][].
+The initial default value is `250`.
 
 * Returns: {number} The current default value of the `autoSelectFamilyAttemptTimeout` option.
 
@@ -1747,8 +1756,8 @@ net.isIPv6('fhqwhgads'); // returns false
 [`net.createConnection(path)`]: #netcreateconnectionpath-connectlistener
 [`net.createConnection(port, host)`]: #netcreateconnectionport-host-connectlistener
 [`net.createServer()`]: #netcreateserveroptions-connectionlistener
-[`net.setDefaultAutoSelectFamily(value)`]: #netsetdefaultautoselectfamilyvalue
-[`net.setDefaultAutoSelectFamilyAttemptTimeout(value)`]: #netsetdefaultautoselectfamilyattempttimeoutvalue
+[`net.getDefaultAutoSelectFamily()`]: #netgetdefaultautoselectfamily
+[`net.getDefaultAutoSelectFamilyAttemptTimeout()`]: #netgetdefaultautoselectfamilyattempttimeout
 [`new net.Socket(options)`]: #new-netsocketoptions
 [`readable.setEncoding()`]: stream.md#readablesetencodingencoding
 [`server.close()`]: #serverclosecallback

lib/internal/errors.js

@@ -1567,6 +1567,8 @@ E('ERR_SOCKET_CLOSED', 'Socket is closed', Error);
 E('ERR_SOCKET_CLOSED_BEFORE_CONNECTION',
   'Socket closed before the connection was established',
   Error);
+E('ERR_SOCKET_CONNECTION_TIMEOUT',
+  'Socket connection timeout', Error);
 E('ERR_SOCKET_DGRAM_IS_CONNECTED', 'Already connected', Error);
 E('ERR_SOCKET_DGRAM_NOT_CONNECTED', 'Not connected', Error);
 E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running', Error);

lib/internal/net.js

@@ -67,7 +67,7 @@ function makeSyncWrite(fd) {
 }
 
 module.exports = {
-  kReinitializeHandle: Symbol('reinitializeHandle'),
+  kReinitializeHandle: Symbol('kReinitializeHandle'),
   isIP,
   isIPv4,
   isIPv6,

lib/net.js

@@ -23,6 +23,7 @@
 
 const {
   ArrayIsArray,
+  ArrayPrototypeIncludes,
   ArrayPrototypeIndexOf,
   ArrayPrototypePush,
   Boolean,
@@ -97,6 +98,7 @@ const {
     ERR_INVALID_HANDLE_TYPE,
     ERR_SERVER_ALREADY_LISTEN,
     ERR_SERVER_NOT_RUNNING,
+    ERR_SOCKET_CONNECTION_TIMEOUT,
     ERR_SOCKET_CLOSED,
     ERR_SOCKET_CLOSED_BEFORE_CONNECTION,
     ERR_MISSING_ARGS,
@@ -127,7 +129,7 @@ let cluster;
 let dns;
 let BlockList;
 let SocketAddress;
-let autoSelectFamilyDefault = getOptionValue('--enable-network-family-autoselection');
+let autoSelectFamilyDefault = getOptionValue('--network-family-autoselection');
 let autoSelectFamilyAttemptTimeoutDefault = 250;
 
 const { clearTimeout, setTimeout } = require('timers');
@@ -1092,6 +1094,11 @@ function internalConnectMultiple(context, canceled) {
 
   // All connections have been tried without success, destroy with error
   if (canceled || context.current === context.addresses.length) {
+    if (context.errors.length === 0) {
+      self.destroy(new ERR_SOCKET_CONNECTION_TIMEOUT());
+      return;
+    }
+
     self.destroy(aggregateErrors(context.errors));
     return;
   }
@@ -1322,6 +1329,7 @@ function lookupAndConnect(self, options) {
         options,
         dnsopts,
         port,
+        localAddress,
         localPort,
         autoSelectFamilyAttemptTimeout,
       );
@@ -1364,7 +1372,9 @@ function lookupAndConnect(self, options) {
   });
 }
 
-function lookupAndConnectMultiple(self, async_id_symbol, lookup, host, options, dnsopts, port, localPort, timeout) {
+function lookupAndConnectMultiple(
+  self, async_id_symbol, lookup, host, options, dnsopts, port, localAddress, localPort, timeout,
+) {
   defaultTriggerAsyncIdScope(self[async_id_symbol], function emitLookup() {
     lookup(host, dnsopts, function emitLookup(err, addresses) {
       // It's possible we were destroyed while looking this up.
@@ -1385,6 +1395,7 @@ function lookupAndConnectMultiple(self, async_id_symbol, lookup, host, options,
       // Filter addresses by only keeping the one which are either IPv4 or IPV6.
       // The first valid address determines which group has preference on the
       // alternate family sorting which happens later.
+      const validAddresses = [[], []];
       const validIps = [[], []];
       let destinations;
       for (let i = 0, l = addresses.length; i < l; i++) {
@@ -1397,12 +1408,19 @@ function lookupAndConnectMultiple(self, async_id_symbol, lookup, host, options,
             destinations = addressType === 6 ? { 6: 0, 4: 1 } : { 4: 0, 6: 1 };
           }
 
-          ArrayPrototypePush(validIps[destinations[addressType]], address);
+          const destination = destinations[addressType];
+
+          // Only try an address once
+          if (!ArrayPrototypeIncludes(validIps[destination], ip)) {
+            ArrayPrototypePush(validAddresses[destination], address);
+            ArrayPrototypePush(validIps[destination], ip);
+          }
         }
       }
 
+
       // When no AAAA or A records are available, fail on the first one
-      if (!validIps[0].length && !validIps[1].length) {
+      if (!validAddresses[0].length && !validAddresses[1].length) {
         const { address: firstIp, family: firstAddressType } = addresses[0];
 
         if (!isIP(firstIp)) {
@@ -1420,16 +1438,36 @@ function lookupAndConnectMultiple(self, async_id_symbol, lookup, host, options,
 
       // Sort addresses alternating families
       const toAttempt = [];
-      for (let i = 0, l = MathMax(validIps[0].length, validIps[1].length); i < l; i++) {
-        if (i in validIps[0]) {
-          ArrayPrototypePush(toAttempt, validIps[0][i]);
+      for (let i = 0, l = MathMax(validAddresses[0].length, validAddresses[1].length); i < l; i++) {
+        if (i in validAddresses[0]) {
+          ArrayPrototypePush(toAttempt, validAddresses[0][i]);
         }
-        if (i in validIps[1]) {
-          ArrayPrototypePush(toAttempt, validIps[1][i]);
+        if (i in validAddresses[1]) {
+          ArrayPrototypePush(toAttempt, validAddresses[1][i]);
         }
       }
 
+      if (toAttempt.length === 1) {
+        debug('connect/multiple: only one address found, switching back to single connection');
+        const { address: ip, family: addressType } = toAttempt[0];
+
+        self._unrefTimer();
+        defaultTriggerAsyncIdScope(
+          self[async_id_symbol],
+          internalConnect,
+          self,
+          ip,
+          port,
+          addressType,
+          localAddress,
+          localPort,
+        );
+
+        return;
+      }
+
       self.autoSelectFamilyAttemptedAddresses = [];
+      debug('connect/multiple: will try the following addresses', toAttempt);
 
       const context = {
         socket: self,
@@ -1543,6 +1581,13 @@ function afterConnect(status, handle, req, readable, writable) {
 }
 
 function afterConnectMultiple(context, status, handle, req, readable, writable) {
+  // One of the connection has completed and correctly dispatched but after timeout, ignore this one
+  if (context[kTimeoutTriggered]) {
+    debug('connect/multiple: ignoring successful but timedout connection to %s:%s', req.address, req.port);
+    handle.close();
+    return;
+  }
+
   const self = context.socket;
 
   // Make sure another connection is not spawned
@@ -1571,13 +1616,6 @@ function afterConnectMultiple(context, status, handle, req, readable, writable)
     return;
   }
 
-  // One of the connection has completed and correctly dispatched but after timeout, ignore this one
-  if (context[kTimeoutTriggered]) {
-    debug('connect/multiple: ignoring successful but timedout connection to %s:%s', req.address, req.port);
-    handle.close();
-    return;
-  }
-
   if (context.current > 1 && self[kReinitializeHandle]) {
     self[kReinitializeHandle](handle);
     handle = self._handle;

src/node_options.cc

@@ -358,10 +358,13 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "returned)",
             &EnvironmentOptions::dns_result_order,
             kAllowedInEnvvar);
-  AddOption("--enable-network-family-autoselection",
-            "Enable network address family autodetection algorithm",
-            &EnvironmentOptions::enable_network_family_autoselection,
-            kAllowedInEnvvar);
+  AddOption("--network-family-autoselection",
+            "Disable network address family autodetection algorithm",
+            &EnvironmentOptions::network_family_autoselection,
+            kAllowedInEnvvar,
+            true);
+  AddAlias("--enable-network-family-autoselection",
+           "--network-family-autoselection");
   AddOption("--enable-source-maps",
             "Source Map V3 support for stack traces",
             &EnvironmentOptions::enable_source_maps,

src/node_options.h

@@ -132,7 +132,7 @@ class EnvironmentOptions : public Options {
   bool frozen_intrinsics = false;
   int64_t heap_snapshot_near_heap_limit = 0;
   std::string heap_snapshot_signal;
-  bool enable_network_family_autoselection = false;
+  bool network_family_autoselection = true;
   uint64_t max_http_header_size = 16 * 1024;
   bool deprecation = true;
   bool force_async_hooks_checks = true;

test/common/index.js

@@ -26,6 +26,7 @@ const process = global.process;  // Some tests tamper with the process global.
 const assert = require('assert');
 const { exec, execSync, spawn, spawnSync } = require('child_process');
 const fs = require('fs');
+const net = require('net');
 // Do not require 'os' until needed so that test-os-checked-function can
 // monkey patch it. If 'os' is required here, that test will fail.
 const path = require('path');
@@ -137,6 +138,14 @@ const isPi = (() => {
 
 const isDumbTerminal = process.env.TERM === 'dumb';
 
+// When using high concurrency or in the CI we need much more time for each connection attempt
+const defaultAutoSelectFamilyAttemptTimeout = platformTimeout(2500);
+// Since this is also used by tools outside of the test suite,
+// make sure setDefaultAutoSelectFamilyAttemptTimeout
+if (typeof net.setDefaultAutoSelectFamilyAttemptTimeout === 'function') {
+  net.setDefaultAutoSelectFamilyAttemptTimeout(platformTimeout(defaultAutoSelectFamilyAttemptTimeout));
+}
+
 const buildType = process.config.target_defaults ?
   process.config.target_defaults.default_configuration :
   'Release';
@@ -886,6 +895,7 @@ const common = {
   canCreateSymLink,
   childShouldThrowAndAbort,
   createZeroFilledFile,
+  defaultAutoSelectFamilyAttemptTimeout,
   expectsError,
   expectWarning,
   gcUntil,

test/internet/test-tls-autoselectfamily-servername.js

@@ -7,12 +7,8 @@ if (!common.hasCrypto) {
   common.skip('missing crypto');
 }
 
-const { setDefaultAutoSelectFamilyAttemptTimeout } = require('net');
 const { connect } = require('tls');
 
-// Some of the windows machines in the CI need more time to establish connection
-setDefaultAutoSelectFamilyAttemptTimeout(common.platformTimeout(common.isWindows ? 1500 : 250));
-
 // Test that TLS connecting works without autoSelectFamily
 {
   const socket = connect({