Skip to content

Commit

Permalink
net: fix family autoselection timeout handling
Browse files Browse the repository at this point in the history 
PR-URL: #47860
Backport-PR-URL: #49016
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
@ShogunPanda @ruyadorno
ShogunPanda authored and ruyadorno committed Aug 14, 2023
1 parent 863bdb7 commit e8289a8
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 16 deletions.
40 changes: 27 additions & 13 deletions lib/net.js
View file
Expand Up @@ -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 = '::';
Expand Down Expand Up @@ -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;

Expand All @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -1419,7 +1424,6 @@ function lookupAndConnectMultiple(self, async_id_symbol, lookup, host, options,
localPort,
timeout,
[kTimeout]: null,
[kTimeoutTriggered]: false,
errors: [],
};

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}

Expand Down
64 changes: 61 additions & 3 deletions test/parallel/test-net-autoselectfamily.js
View file
Expand Up @@ -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');

Expand All @@ -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);
}));
Expand Down Expand Up @@ -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 }) {
Expand Down

0 comments on commit e8289a8

Please sign in to comment.