net: fix family autoselection timeout handling

PR-URL: nodejs#47860 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
juanarbol · Jul 17, 2023 · 6ef172f · 6ef172f
1 parent b209f20
commit 6ef172f
Show file tree

Hide file tree

Showing 2 changed files with 88 additions and 16 deletions.
diff --git a/lib/net.js b/lib/net.js
@@ -135,7 +135,6 @@ let autoSelectFamilyDefault = getOptionValue('--enable-network-family-autoselect
 
 const { clearTimeout, setTimeout } = require('timers');
 const { kTimeout } = require('internal/timers');
-const kTimeoutTriggered = Symbol('kTimeoutTriggered');
 
 const DEFAULT_IPV4_ADDR = '0.0.0.0';
 const DEFAULT_IPV6_ADDR = '::';
@@ -1093,9 +1092,11 @@ function internalConnectMultiple(context) {
     return;
   }
 
+
+  const current = context.current++;
+  const handle = current === 0 ? self._handle : new TCP(TCPConstants.SOCKET);
   const { localPort, port, flags } = context;
-  const { address, family: addressType } = context.addresses[context.current++];
-  const handle = new TCP(TCPConstants.SOCKET);
+  const { address, family: addressType } = context.addresses[current];
   let localAddress;
   let err;
 
@@ -1120,7 +1121,7 @@ function internalConnectMultiple(context) {
   }
 
   const req = new TCPConnectWrap();
-  req.oncomplete = FunctionPrototypeBind(afterConnectMultiple, undefined, context);
+  req.oncomplete = FunctionPrototypeBind(afterConnectMultiple, undefined, context, current);
   req.address = address;
   req.port = port;
   req.localAddress = localAddress;
@@ -1147,8 +1148,12 @@ function internalConnectMultiple(context) {
     return;
   }
 
-  // If the attempt has not returned an error, start the connection timer
-  context[kTimeout] = setTimeout(internalConnectMultipleTimeout, context.timeout, context, req);
+  if (current < context.addresses.length - 1) {
+    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);
+  }
 }
 
 Socket.prototype.connect = function(...args) {
@@ -1419,7 +1424,6 @@ function lookupAndConnectMultiple(self, async_id_symbol, lookup, host, options,
         localPort,
         timeout,
         [kTimeout]: null,
-        [kTimeoutTriggered]: false,
         errors: [],
       };
 
@@ -1522,12 +1526,20 @@ function afterConnect(status, handle, req, readable, writable) {
   }
 }
 
-function afterConnectMultiple(context, status, handle, req, readable, writable) {
-  const self = context.socket;
-
+function afterConnectMultiple(context, current, status, handle, req, readable, writable) {
   // Make sure another connection is not spawned
   clearTimeout(context[kTimeout]);
 
+  // One of the connection has completed and correctly dispatched but after timeout, ignore this one
+  if (status === 0 && current !== context.current - 1) {
+    debug('connect/multiple: ignoring successful but timedout connection to %s:%s', req.address, req.port);
+    handle.close();
+    return;
+  }
+
+  const self = context.socket;
+
+
   // Some error occurred, add to the list of exceptions
   if (status !== 0) {
     let details;
@@ -1552,7 +1564,7 @@ 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]) {
+  if (status === 0 && current !== context.current - 1) {
     debug('connect/multiple: ignoring successful but timedout connection to %s:%s', req.address, req.port);
     handle.close();
     return;
@@ -1578,8 +1590,10 @@ function afterConnectMultiple(context, status, handle, req, readable, writable)
   afterConnect(status, handle, req, readable, writable);
 }
 
-function internalConnectMultipleTimeout(context, req) {
-  context[kTimeoutTriggered] = true;
+function internalConnectMultipleTimeout(context, req, handle) {
+  debug('connect/multiple: connection to %s:%s timed out', req.address, req.port);
+  req.oncomplete = undefined;
+  handle.close();
   internalConnectMultiple(context);
 }
 

diff --git a/test/parallel/test-net-autoselectfamily.js b/test/parallel/test-net-autoselectfamily.js
@@ -36,7 +36,15 @@ function _lookup(resolver, hostname, options, cb) {
   });
 }
 
-function createDnsServer(ipv6Addr, ipv4Addr, cb) {
+function createDnsServer(ipv6Addrs, ipv4Addrs, cb) {
+  if (!Array.isArray(ipv6Addrs)) {
+    ipv6Addrs = [ipv6Addrs];
+  }
+
+  if (!Array.isArray(ipv4Addrs)) {
+    ipv4Addrs = [ipv4Addrs];
+  }
+
   // Create a DNS server which replies with a AAAA and a A record for the same host
   const socket = dgram.createSocket('udp4');
 
@@ -49,8 +57,8 @@ function createDnsServer(ipv6Addr, ipv4Addr, cb) {
       id: parsed.id,
       questions: parsed.questions,
       answers: [
-        { type: 'AAAA', address: ipv6Addr, ttl: 123, domain: 'example.org' },
-        { type: 'A', address: ipv4Addr, ttl: 123, domain: 'example.org' },
+        ...ipv6Addrs.map((address) => ({ type: 'AAAA', address, ttl: 123, domain: 'example.org' })),
+        ...ipv4Addrs.map((address) => ({ type: 'A', address, ttl: 123, domain: 'example.org' })),
       ]
     }), port, address);
   }));
@@ -106,6 +114,56 @@ function createDnsServer(ipv6Addr, ipv4Addr, cb) {
   }));
 }
 
+// Test that only the last successful connection is established.
+{
+  createDnsServer(
+    '::1',
+    ['104.20.22.46', '104.20.23.46', '127.0.0.1'],
+    common.mustCall(function({ dnsServer, lookup }) {
+      const ipv4Server = createServer((socket) => {
+        socket.on('data', common.mustCall(() => {
+          socket.write('response-ipv4');
+          socket.end();
+        }));
+      });
+
+      ipv4Server.listen(0, '127.0.0.1', common.mustCall(() => {
+        const port = ipv4Server.address().port;
+
+        const connection = createConnection({
+          host: 'example.org',
+          port: port,
+          lookup,
+          autoSelectFamily: true,
+          autoSelectFamilyAttemptTimeout,
+        });
+
+        let response = '';
+        connection.setEncoding('utf-8');
+
+        connection.on('ready', common.mustCall(() => {
+          assert.deepStrictEqual(
+            connection.autoSelectFamilyAttemptedAddresses,
+            [`::1:${port}`, `104.20.22.46:${port}`, `104.20.23.46:${port}`, `127.0.0.1:${port}`]
+          );
+        }));
+
+        connection.on('data', (chunk) => {
+          response += chunk;
+        });
+
+        connection.on('end', common.mustCall(() => {
+          assert.strictEqual(response, 'response-ipv4');
+          ipv4Server.close();
+          dnsServer.close();
+        }));
+
+        connection.write('request');
+      }));
+    })
+  );
+}
+
 // Test that IPV4 is NOT reached if IPV6 is reachable
 if (common.hasIPv6) {
   createDnsServer('::1', '127.0.0.1', common.mustCall(function({ dnsServer, lookup }) {