From 16ab933e53546d2ce1b3483b5d31f5718e39901f Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Thu, 15 Jun 2023 10:44:03 +0200 Subject: [PATCH] net: improve network family autoselection handle handling --- lib/net.js | 110 ++++++++++++------ ...est-https-autoselectfamily-slow-timeout.js | 2 +- ...test-net-autoselectfamily-timeout-close.js | 27 +++++ ...est-tls-autoselectfamily-backing-socket.js | 22 ++++ 4 files changed, 125 insertions(+), 36 deletions(-) create mode 100644 test/internet/test-net-autoselectfamily-timeout-close.js create mode 100644 test/internet/test-tls-autoselectfamily-backing-socket.js diff --git a/lib/net.js b/lib/net.js index e3591b72540ea8..c1fd13ca19d9b5 100644 --- a/lib/net.js +++ b/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); diff --git a/test/internet/test-https-autoselectfamily-slow-timeout.js b/test/internet/test-https-autoselectfamily-slow-timeout.js index ea8f1374c01a23..e7c55df9c8da40 100644 --- a/test/internet/test-https-autoselectfamily-slow-timeout.js +++ b/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); diff --git a/test/internet/test-net-autoselectfamily-timeout-close.js b/test/internet/test-net-autoselectfamily-timeout-close.js new file mode 100644 index 00000000000000..b93bbc546d24e9 --- /dev/null +++ b/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); + })); +} diff --git a/test/internet/test-tls-autoselectfamily-backing-socket.js b/test/internet/test-tls-autoselectfamily-backing-socket.js new file mode 100644 index 00000000000000..9e523fd7c90fa8 --- /dev/null +++ b/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 }); +}