diff --git a/lib/net.js b/lib/net.js index 445a7d59f8c..8029026c149 100644 --- a/lib/net.js +++ b/lib/net.js @@ -376,6 +376,13 @@ function closeSocketHandle(self, isException, isCleanupPending = false) { const kBytesRead = Symbol('kBytesRead'); const kBytesWritten = Symbol('kBytesWritten'); const kSetTOS = Symbol('kSetTOS'); +// Generation token for a Socket's connection lifecycle. Async work started by +// a connect() attempt (e.g. a DNS lookup) captures the current generation; the +// generation is advanced once per lifecycle, in _destroy, so stale async work +// from a destroyed lifecycle cannot drive a newer lifecycle established by a +// later connect() (e.g. reconnect after 'close'). Multiple connect() calls +// within one undestroyed lifecycle share a generation. +const kConnectGeneration = Symbol('kConnectGeneration'); // Marks a Socket whose handle is an adopted, already-bound BoundSocket. const kBoundSource = Symbol('kBoundSource'); @@ -1100,6 +1107,9 @@ Socket.prototype._destroy = function(exception, cb) { debug('destroy'); this.connecting = false; + // The connection lifecycle ends here; invalidate async work (e.g. a DNS + // lookup) still in flight so it cannot drive a newer lifecycle. + this[kConnectGeneration] = (this[kConnectGeneration] || 0) + 1; for (let s = this; s !== null; s = s._parent) { clearTimeout(s[kTimeout]); @@ -1415,6 +1425,13 @@ function internalConnectMultiple(context, canceled) { clearTimeout(context[kTimeout]); const self = context.socket; + // The connection lifecycle that created this context may have been destroyed + // and a newer connect() established; a stale context must not drive the new + // lifecycle's handle. + if (self[kConnectGeneration] !== context.connectGeneration) { + return; + } + // We were requested to abort. Stop all operations if (self._aborted) { return; @@ -1778,6 +1795,8 @@ function lookupAndConnect(self, options) { return; } + const connectGeneration = self[kConnectGeneration]; + defaultTriggerAsyncIdScope(self[async_id_symbol], function() { lookup(host, dnsopts, function emitLookup(err, ip, addressType) { self.emit('lookup', err, ip, addressType, host); @@ -1785,7 +1804,9 @@ function lookupAndConnect(self, options) { // It's possible we were destroyed while looking this up. // XXX it would be great if we could cancel the promise returned by // the look up. - if (!self.connecting) return; + // A stale callback may also arrive after the attempt was destroyed and a + // newer connect() attempt was started; it must not drive the new attempt. + if (!self.connecting || self[kConnectGeneration] !== connectGeneration) return; if (err) { // net.createConnection() creates a net.Socket object and immediately @@ -1815,12 +1836,16 @@ function lookupAndConnect(self, options) { function lookupAndConnectMultiple( self, async_id_symbol, lookup, host, options, dnsopts, port, localAddress, localPort, timeout, ) { + const connectGeneration = self[kConnectGeneration]; + defaultTriggerAsyncIdScope(self[async_id_symbol], function emitLookup() { lookup(host, dnsopts, function emitLookup(err, addresses) { // It's possible we were destroyed while looking this up. // XXX it would be great if we could cancel the promise returned by // the look up. - if (!self.connecting) { + // A stale callback may also arrive after the attempt was destroyed and a + // newer connect() attempt was started; it must not drive the new attempt. + if (!self.connecting || self[kConnectGeneration] !== connectGeneration) { return; } else if (err) { self.emit('lookup', err, undefined, undefined, host); @@ -1843,7 +1868,7 @@ function lookupAndConnectMultiple( const { address: ip, family: addressType } = address; self.emit('lookup', err, ip, addressType, host); // It's possible we were destroyed while looking this up. - if (!self.connecting) { + if (!self.connecting || self[kConnectGeneration] !== connectGeneration) { return; } if (isIP(ip) && (addressType === 4 || addressType === 6)) { @@ -1914,6 +1939,7 @@ function lookupAndConnectMultiple( socket: self, addresses: toAttempt, current: 0, + connectGeneration, port, localPort, timeout, @@ -2087,6 +2113,14 @@ function afterConnectMultiple(context, current, status, handle, req, readable, w const self = context.socket; + // The connection lifecycle that created this context may have been destroyed + // and a newer connect() established; drop the stale completion, closing the + // completed handle so it cannot leak. + if (self[kConnectGeneration] !== context.connectGeneration) { + handle.close(); + return; + } + // Some error occurred, add to the list of exceptions if (status !== 0) { const ex = createConnectionError(req, status); @@ -2115,6 +2149,14 @@ function afterConnectMultiple(context, current, status, handle, req, readable, w function internalConnectMultipleTimeout(context, req, handle) { debug('connect/multiple: connection to %s:%s timed out', req.address, req.port); + + // The connection lifecycle that created this context may have been destroyed + // and a newer connect() established; drop the stale timeout so it cannot + // emit a spurious connectionAttemptTimeout or drive further attempts. + if (context.socket[kConnectGeneration] !== context.connectGeneration) { + return; + } + context.socket.emit('connectionAttemptTimeout', req.address, req.port, req.addressType); req.oncomplete = undefined; diff --git a/test/parallel/test-net-connect-immediate-destroy-reconnect.js b/test/parallel/test-net-connect-immediate-destroy-reconnect.js new file mode 100644 index 00000000000..756c452238b --- /dev/null +++ b/test/parallel/test-net-connect-immediate-destroy-reconnect.js @@ -0,0 +1,114 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); + +// Regression test for https://github.com/nodejs/node/issues/55519. +// +// Destroying a socket while its DNS lookup is still in flight and then +// reconnecting on 'close' must not let the stale lookup callback from the +// destroyed attempt drive internalConnect on the new attempt's handle. Doing +// so would issue a second connect on the same handle and fail the connection +// with EALREADY (EINVAL on Windows). +// +// The flow is driven deterministically: the custom lookup captures the +// callbacks of both attempts and the stale (attempt 1) callback is invoked +// before the callback of the current (attempt 2) attempt. + +const cases = [ + { autoSelectFamily: false }, + { autoSelectFamily: true }, +]; + +function runCase(options, done) { + let connected = false; + let accepted = false; + + const watchdog = setTimeout(() => { + console.error(`test-case timed out: ${JSON.stringify(options)}`); + process.exit(1); + }, 10_000); + + const socket = new net.Socket(); + + const finish = common.mustCall(() => { + clearTimeout(watchdog); + socket.destroy(); + server.close(); + done(); + }); + + // Teardown only after both the client has connected and the server has + // accepted the connection, so the accept callback is never dropped by an + // early server.close(). + const server = net.createServer(common.mustCall(() => { + accepted = true; + if (connected) { + finish(); + } + })); + + server.listen(0, common.localhostIPv4, common.mustCall(() => { + const port = server.address().port; + const lookupCalls = []; + let reconnected = false; + let connectionAttempts = 0; + + function lookup(host, dnsopts, cb) { + lookupCalls.push({ dnsopts, cb }); + } + + socket.on('connectionAttempt', () => { + connectionAttempts++; + }); + + socket.on('connect', common.mustCall(() => { + // Only the current (second) attempt may have connected: the stale + // callback must not have started a connect on the new attempt's handle. + assert.strictEqual(connectionAttempts, 1); + connected = true; + if (accepted) { + finish(); + } + })); + + socket.on('error', common.mustNotCall()); + + socket.on('close', common.mustCallAtLeast(() => { + if (reconnected) { + return; + } + reconnected = true; + // Start a new connection attempt; the lookup callback of the first + // attempt is still pending. + socket.connect({ host: 'host.example', port, lookup, ...options }); + assert.strictEqual(lookupCalls.length, 2); + + const fire = (call) => { + if (call.dnsopts.all === true) { + call.cb(null, [{ address: common.localhostIPv4, family: 4 }]); + } else { + call.cb(null, common.localhostIPv4, 4); + } + }; + // Invoke the stale callback first, then the current attempt's callback. + // The stale callback must be ignored. + fire(lookupCalls[0]); + fire(lookupCalls[1]); + })); + + socket.connect({ host: 'host.example', port, lookup, ...options }); + socket.destroy(); + })); +} + +let index = 0; +function next() { + if (index >= cases.length) { + return; + } + runCase(cases[index++], next); +} + +next();