From c8b5f22bf37c38d8a288cb3a4aff19d39d99dcb3 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Mon, 13 Feb 2023 10:03:50 +0100 Subject: [PATCH] net: rework autoSelectFamily implementation --- lib/_tls_wrap.js | 11 ----- lib/internal/net.js | 1 - lib/net.js | 46 ++++++++++--------- src/tcp_wrap.cc | 12 +++++ src/tcp_wrap.h | 1 + .../test-http2-ping-settings-heapdump.js | 10 +++- test/parallel/test-net-dns-custom-lookup.js | 19 ++++++-- test/parallel/test-net-dns-lookup.js | 4 +- test/parallel/test-net-options-lookup.js | 6 ++- test/parallel/test-net-server-reset.js | 18 +++----- 10 files changed, 76 insertions(+), 52 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 78fe3191dc86d5..d0fc9ceeeb5340 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -54,7 +54,6 @@ const EE = require('events'); const net = require('net'); const tls = require('tls'); const common = require('_tls_common'); -const { kWrapConnectedHandle } = require('internal/net'); const JSStreamSocket = require('internal/js_stream_socket'); const { Buffer } = require('buffer'); let debug = require('internal/util/debuglog').debuglog('tls', (fn) => { @@ -633,16 +632,6 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle) { return res; }; -TLSSocket.prototype[kWrapConnectedHandle] = function(handle) { - this._handle = this._wrapHandle(null, handle); - this.ssl = this._handle; - this._init(); - - if (this._tlsOptions.enableTrace) { - this._handle.enableTrace(); - } -}; - // This eliminates a cyclic reference to TLSWrap // Ref: https://github.com/nodejs/node/commit/f7620fb96d339f704932f9bb9a0dceb9952df2d4 function defineHandleReading(socket, handle) { diff --git a/lib/internal/net.js b/lib/internal/net.js index 35e2a03706a675..625377acd57caa 100644 --- a/lib/internal/net.js +++ b/lib/internal/net.js @@ -67,7 +67,6 @@ function makeSyncWrite(fd) { } module.exports = { - kWrapConnectedHandle: Symbol('wrapConnectedHandle'), isIP, isIPv4, isIPv6, diff --git a/lib/net.js b/lib/net.js index 6f821651d1ce29..944aa9eb50a87e 100644 --- a/lib/net.js +++ b/lib/net.js @@ -42,7 +42,6 @@ let debug = require('internal/util/debuglog').debuglog('net', (fn) => { debug = fn; }); const { - kWrapConnectedHandle, isIP, isIPv4, isIPv6, @@ -53,7 +52,8 @@ const assert = require('internal/assert'); const { UV_EADDRINUSE, UV_EINVAL, - UV_ENOTCONN + UV_ENOTCONN, + UV_ECANCELED } = internalBinding('uv'); const { Buffer } = require('buffer'); @@ -1064,36 +1064,45 @@ function internalConnect( } -function internalConnectMultiple(context) { +function internalConnectMultiple(context, canceled) { clearTimeout(context[kTimeout]); const self = context.socket; - assert(self.connecting); // All connections have been tried without success, destroy with error - if (context.current === context.addresses.length) { + if (canceled || context.current === context.addresses.length) { self.destroy(aggregateErrors(context.errors)); return; } + assert(self.connecting); + + // Reset the TCP handle when trying other addresses + if (context.current > 0) { + if (self?.[kHandle]?._parent) { + self[kHandle]._parent.reinitialize(); + } else { + self._handle.reinitialize(); + } + } + const { localPort, port, flags } = context; const { address, family: addressType } = context.addresses[context.current++]; - const handle = new TCP(TCPConstants.SOCKET); let localAddress; let err; 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); @@ -1101,6 +1110,8 @@ function internalConnectMultiple(context) { } } + debug('connect/multiple: attempting to connect to %s:%d (addressType: %d)', address, port, addressType); + const req = new TCPConnectWrap(); req.oncomplete = FunctionPrototypeBind(afterConnectMultiple, undefined, context); req.address = address; @@ -1111,9 +1122,9 @@ function internalConnectMultiple(context) { 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) { @@ -1337,6 +1348,8 @@ function lookupAndConnectMultiple(self, async_id_symbol, lookup, host, options, if (!self.connecting) { return; } else if (err) { + self.emit('lookup', err, undefined, undefined, host); + // net.createConnection() creates a net.Socket object and immediately // calls net.Socket.connect() on it (that's us). There are no event // listeners registered yet so defer the error event to the next tick. @@ -1529,7 +1542,7 @@ function afterConnectMultiple(context, status, handle, req, readable, writable) ArrayPrototypePush(context.errors, ex); // Try the next address - internalConnectMultiple(context); + internalConnectMultiple(context, status === UV_ECANCELED); return; } @@ -1540,15 +1553,6 @@ function afterConnectMultiple(context, status, handle, req, readable, writable) return; } - // Perform initialization sequence on the handle, then move on with the regular callback - self._handle = handle; - initSocketHandle(self); - - if (self[kWrapConnectedHandle]) { - self[kWrapConnectedHandle](handle); - initSocketHandle(self); // This is called again to initialize the TLSWrap - } - if (hasObserver('net')) { startPerf( self, diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index baa7618dfa0743..2ff7308fefe56c 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -104,6 +104,7 @@ void TCPWrap::Initialize(Local target, SetProtoMethod(isolate, t, "setNoDelay", SetNoDelay); SetProtoMethod(isolate, t, "setKeepAlive", SetKeepAlive); SetProtoMethod(isolate, t, "reset", Reset); + SetProtoMethod(isolate, t, "reinitialize", Reinitialize); #ifdef _WIN32 SetProtoMethod(isolate, t, "setSimultaneousAccepts", SetSimultaneousAccepts); @@ -142,6 +143,7 @@ void TCPWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(SetNoDelay); registry->Register(SetKeepAlive); registry->Register(Reset); + registry->Register(Reinitialize); #ifdef _WIN32 registry->Register(SetSimultaneousAccepts); #endif @@ -181,6 +183,16 @@ TCPWrap::TCPWrap(Environment* env, Local object, ProviderType provider) // Suggestion: uv_tcp_init() returns void. } +void TCPWrap::Reinitialize(const FunctionCallbackInfo& args) { + TCPWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, + args.Holder(), + args.GetReturnValue().Set(UV_EBADF)); + + Environment* env = wrap->env(); + int r = uv_tcp_init(env->event_loop(), &wrap->handle_); + CHECK_EQ(r, 0); +} void TCPWrap::SetNoDelay(const FunctionCallbackInfo& args) { TCPWrap* wrap; diff --git a/src/tcp_wrap.h b/src/tcp_wrap.h index a4684b65d24934..3c116667e708be 100644 --- a/src/tcp_wrap.h +++ b/src/tcp_wrap.h @@ -72,6 +72,7 @@ class TCPWrap : public ConnectionWrap { ProviderType provider); static void New(const v8::FunctionCallbackInfo& args); + static void Reinitialize(const v8::FunctionCallbackInfo& args); static void SetNoDelay(const v8::FunctionCallbackInfo& args); static void SetKeepAlive(const v8::FunctionCallbackInfo& args); static void Bind(const v8::FunctionCallbackInfo& args); diff --git a/test/parallel/test-http2-ping-settings-heapdump.js b/test/parallel/test-http2-ping-settings-heapdump.js index 7d27310700c7a8..775110c098607b 100644 --- a/test/parallel/test-http2-ping-settings-heapdump.js +++ b/test/parallel/test-http2-ping-settings-heapdump.js @@ -11,6 +11,14 @@ const v8 = require('v8'); // after it is destroyed, either because they are detached from it or have been // destroyed themselves. +// We use an higher autoSelectFamilyAttemptTimeout in this test as the v8.getHeapSnapshot().resume() +// will slow the connection flow and we don't want the second connection attempt to start. +let autoSelectFamilyAttemptTimeout = common.platformTimeout(1000); +if (common.isWindows) { + // Some of the windows machines in the CI need more time to establish connection + autoSelectFamilyAttemptTimeout = common.platformTimeout(10000); +} + for (const variant of ['ping', 'settings']) { const server = http2.createServer(); server.on('session', common.mustCall((session) => { @@ -30,7 +38,7 @@ for (const variant of ['ping', 'settings']) { })); server.listen(0, common.mustCall(() => { - const client = http2.connect(`http://localhost:${server.address().port}`, + const client = http2.connect(`http://localhost:${server.address().port}`, { autoSelectFamilyAttemptTimeout }, common.mustCall()); client.on('error', (err) => { // We destroy the session so it's possible to get ECONNRESET here. diff --git a/test/parallel/test-net-dns-custom-lookup.js b/test/parallel/test-net-dns-custom-lookup.js index a7c05c82b95419..e1e425abd0beb8 100644 --- a/test/parallel/test-net-dns-custom-lookup.js +++ b/test/parallel/test-net-dns-custom-lookup.js @@ -26,13 +26,22 @@ function check(addressType, cb) { function lookup(host, dnsopts, cb) { dnsopts.family = addressType; + if (addressType === 4) { process.nextTick(function() { - cb(null, common.localhostIPv4, 4); + if (dnsopts.all) { + cb(null, [{ address: common.localhostIPv4, family: 4 }]); + } else { + cb(null, common.localhostIPv4, 4); + } }); } else { process.nextTick(function() { - cb(null, '::1', 6); + if (dnsopts.all) { + cb(null, [{ address: '::1', family: 6 }]); + } else { + cb(null, '::1', 6); + } }); } } @@ -48,7 +57,11 @@ check(4, function() { host: 'localhost', port: 80, lookup(host, dnsopts, cb) { - cb(null, undefined, 4); + if (dnsopts.all) { + cb(null, [{ address: undefined, family: 4 }]); + } else { + cb(null, undefined, 4); + } } }).on('error', common.expectsError({ code: 'ERR_INVALID_IP_ADDRESS' })); } diff --git a/test/parallel/test-net-dns-lookup.js b/test/parallel/test-net-dns-lookup.js index 8c7bbef128d7b1..8ef0382ae1a816 100644 --- a/test/parallel/test-net-dns-lookup.js +++ b/test/parallel/test-net-dns-lookup.js @@ -31,10 +31,10 @@ const server = net.createServer(function(client) { server.listen(0, common.mustCall(function() { net.connect(this.address().port, 'localhost') - .on('lookup', common.mustCall(function(err, ip, type, host) { + .on('lookup', common.mustCallAtLeast(function(err, ip, type, host) { assert.strictEqual(err, null); assert.match(ip, /^(127\.0\.0\.1|::1)$/); assert.match(type.toString(), /^(4|6)$/); assert.strictEqual(host, 'localhost'); - })); + }, 1)); })); diff --git a/test/parallel/test-net-options-lookup.js b/test/parallel/test-net-options-lookup.js index 0341a9f8d546b5..9a7ab00de96fba 100644 --- a/test/parallel/test-net-options-lookup.js +++ b/test/parallel/test-net-options-lookup.js @@ -36,7 +36,11 @@ function connectDoesNotThrow(input) { { // Verify that an error is emitted when an invalid address family is returned. const s = connectDoesNotThrow((host, options, cb) => { - cb(null, '127.0.0.1', 100); + if (options.all) { + cb(null, [{ address: '127.0.0.1', family: 100 }]); + } else { + cb(null, '127.0.0.1', 100); + } }); s.on('error', common.expectsError({ diff --git a/test/parallel/test-net-server-reset.js b/test/parallel/test-net-server-reset.js index ea78cd2743298e..3f9c049f3c3c36 100644 --- a/test/parallel/test-net-server-reset.js +++ b/test/parallel/test-net-server-reset.js @@ -20,17 +20,11 @@ server.on('close', common.mustCall()); assert.strictEqual(server, server.listen(0, () => { net.createConnection(server.address().port) - .on('error', common.mustCall( - common.expectsError({ - code: 'ECONNRESET', - name: 'Error' - })) - ); + .on('error', common.mustCall((error) => { + assert.strictEqual(error.code, 'ECONNRESET'); + })); net.createConnection(server.address().port) - .on('error', common.mustCall( - common.expectsError({ - code: 'ECONNRESET', - name: 'Error' - })) - ); + .on('error', common.mustCall((error) => { + assert.strictEqual(error.code, 'ECONNRESET'); + })); }));