From c96ef056a632dfd00efaa0a2ac0861d711721eb6 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Thu, 11 May 2023 09:14:03 -0700 Subject: [PATCH] net: fix family autoselection timeout handling PR-URL: https://github.com/nodejs/node/pull/47860 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/net.js | 40 +++++++++----- test/parallel/test-net-autoselectfamily.js | 64 +++++++++++++++++++++- 2 files changed, 88 insertions(+), 16 deletions(-) diff --git a/lib/net.js b/lib/net.js index a72e48c96a6db4..5866235ba7f38a 100644 --- 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 index d664d8c15b9572..43ae91f61c1879 100644 --- 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 }) {