From f4ffcb895abad3595d119074e3424cd8f3f27e2c Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Sat, 11 Feb 2023 22:03:54 +0100 Subject: [PATCH] net: reuse handle with resetting --- doc/api/net.md | 1 + lib/_tls_wrap.js | 11 ----- lib/internal/net.js | 1 - lib/net.js | 44 ++++++++++--------- src/tcp_wrap.cc | 12 +++++ src/tcp_wrap.h | 1 + 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 +++----- .../parallel/test-tls-connect-hints-option.js | 2 +- test/sequential/test-http-econnrefused.js | 2 +- .../test-net-better-error-messages-port.js | 9 ++-- 13 files changed, 73 insertions(+), 57 deletions(-) diff --git a/doc/api/net.md b/doc/api/net.md index 51a74891447c626..112251e2bda3317 100644 --- a/doc/api/net.md +++ b/doc/api/net.md @@ -941,6 +941,7 @@ For TCP connections, available `options` are: option before timing out and trying the next address. Ignored if the `family` option is not `0` or if `localAddress` is set. Connection errors are not emitted if at least one connection succeeds. + If all connections attempts fails a single `AggregateError` with all failed attempts is emitted. **Default:** initially `true`, but it can be changed at runtime using [`net.setDefaultAutoSelectFamily(value)`][] or via the command line option `--no-network-family-autoselection`. * `autoSelectFamilyAttemptTimeout` {number}: The amount of time in milliseconds to wait diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 78fe3191dc86d5a..d0fc9ceeeb53405 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 35e2a03706a6754..625377acd57caa8 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 e62cc7005dc1814..7bb15925ea3d0ef 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); @@ -1111,9 +1120,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 +1346,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 +1540,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 +1551,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 baa7618dfa07437..ffce7d4abdedc5c 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 a4684b65d24934d..3c116667e708be2 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-net-dns-custom-lookup.js b/test/parallel/test-net-dns-custom-lookup.js index a7c05c82b95419a..e1e425abd0beb89 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 8c7bbef128d7b16..8ef0382ae1a8161 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 0341a9f8d546b5b..9a7ab00de96fbad 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 ea78cd2743298eb..3f9c049f3c3c361 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'); + })); })); diff --git a/test/parallel/test-tls-connect-hints-option.js b/test/parallel/test-tls-connect-hints-option.js index 4653e0a1418bbda..6abcf19402efcbb 100644 --- a/test/parallel/test-tls-connect-hints-option.js +++ b/test/parallel/test-tls-connect-hints-option.js @@ -25,7 +25,7 @@ tls.connect({ port: 42, lookup: common.mustCall((host, options) => { assert.strictEqual(host, 'localhost'); - assert.deepStrictEqual(options, { family: undefined, hints }); + assert.deepStrictEqual(options, { family: undefined, hints, all: true }); }), hints }); diff --git a/test/sequential/test-http-econnrefused.js b/test/sequential/test-http-econnrefused.js index 7f8c2cee56c4c8f..2a8d30c7c8527b0 100644 --- a/test/sequential/test-http-econnrefused.js +++ b/test/sequential/test-http-econnrefused.js @@ -135,7 +135,7 @@ function ping() { console.log(`Error making ping req: ${error}`); hadError = true; assert.ok(!gotEnd); - afterPing(error.message); + afterPing(error.errors[0].message); }); } diff --git a/test/sequential/test-net-better-error-messages-port.js b/test/sequential/test-net-better-error-messages-port.js index c21427ee3951397..1d597e60cc69865 100644 --- a/test/sequential/test-net-better-error-messages-port.js +++ b/test/sequential/test-net-better-error-messages-port.js @@ -7,8 +7,9 @@ const c = net.createConnection(common.PORT); c.on('connect', common.mustNotCall()); -c.on('error', common.mustCall(function(e) { - assert.strictEqual(e.code, 'ECONNREFUSED'); - assert.strictEqual(e.port, common.PORT); - assert.match(e.address, /^(127\.0\.0\.1|::1)$/); +c.on('error', common.mustCall(function(error) { + const failedAttempt = error.errors[0]; + assert.strictEqual(failedAttempt.code, 'ECONNREFUSED'); + assert.strictEqual(failedAttempt.port, common.PORT); + assert.match(failedAttempt.address, /^(127\.0\.0\.1|::1)$/); }));