Reduce TLS 'close' event listener warnings caused by autoSelectFamily

[pimterry]

There's been quite a few reports of TLSSocket MaxListenersExceededWarning errors for 'close' events in Node v20, particularly with npm (npm/cli#6763 (comment), #49955, #49337, #49269). I've also seen this myself in my node code when using TLS sockets heavily after updating to Node v20.

This is intermittent and hard to reliably reproduce. That said, this PR fixes a clear small bug that adds duplicate close listeners to many TLS sockets, which is probably a significant factor here (could be the whole problem, hard to say). Anecdotally, with this change testing locally I haven't seen any more of these messages.

The specific line changed here is referenced by quite a few of the reports and in my own output, here's an example stacktrace:

(node:19864) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [TLSSocket]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:588:17)
    at TLSSocket.addListener (node:events:606:10)
    at Readable.on (node:internal/streams/readable:904:35)
    at TLSSocket._wrapHandle (node:_tls_wrap:706:8)
    at TLSSocket.reinitializeHandle (node:_tls_wrap:715:22)
    at internalConnectMultiple (node:net:1123:30)
    at Timeout.internalConnectMultipleTimeout (node:net:1687:3)
    at listOnTimeout (node:internal/timers:575:11)
    at process.processTimers (node:internal/timers:514:7)

In this specific case, reinitializeHandle is called as part of the autoselect family logic (notably enabled by default in Node v20) where sockets are reinitialized repeatedly to test different addresses.

This calls _wrapHandle, which adds a close listener to this (the TLS socket) every time. Since this is called repeatedly when testing addresses, that results in adding one additional close listener for the same callback for every reinitialization (i.e. for each detect IPv4 or IPV6 address of the target host). I think there's no functional impact to this bug (it effectively calls destroy multiple times on close, which does nothing after the first call) but it does potentially add lots of unnecessary listeners.

This PR fixes that, so it now only registers this close listener if one is not already present (I think listenerCount is the best way to do that, but better alternatives are welcome).

[lpinca]

It would be nice to add a test.

cc: @ShogunPanda

[bnoordhuis]

Change itself LGTM but such defensive coding suggests the whole reinitializeHandle approach should be replaced by something better, cc @ShogunPanda. A well-designed code base doesn't need to defend against itself.

[pimterry]

It would be nice to add a test.

@lpinca Agreed, but it's difficult to do so nicely for an intermittent issue like this, suggestions welcome if you think there's a good approach that won't be too fragile.

That at least one close listener is set is already covered - test-tls-connect-abort-controller checks various TLS socket shutdown cases and if the on('close'...) is commented out entirely then it times out every time.

[ShogunPanda]

Change itself LGTM but such defensive coding suggests the whole reinitializeHandle approach should be replaced by something better, cc @ShogunPanda. A well-designed code base doesn't need to defend against itself.

I agree on that. When implementing it i noticed that TLS interacts with sockets initialization in many place and different ways. So reinitalizing was the only approach I found. Can you help me understand what is improvement you are proposing?

[ShogunPanda]

It would be nice to add a test.

cc: @ShogunPanda

Yeah, I also agree, but's it's very hard to add when it's pretty impossible to reproduce it consistently.

The entire network connection thing is at the moment really complicated. IMHO the network autoselection feature only exposed a possible weak area in the codebase which is very hard to extend/interact with.

[lpinca]

Something like this should work

// Flags: --expose-internals

'use strict';

const common = require('../common');

if (!common.hasCrypto) {
  common.skip('missing crypto');
}

const events = require('events');
const fixtures = require('../common/fixtures');
const tls = require('tls');
const { kReinitializeHandle } = require('internal/net');

process.on('warning', common.mustNotCall());

const server = tls.createServer({
  key: fixtures.readKey('agent1-key.pem'),
  cert: fixtures.readKey('agent1-cert.pem')
});

server.listen(0, common.mustCall(function() {
  const socket = tls.connect({
    port: this.address().port,
    rejectUnauthorized: false
  });

  socket.on('secureConnect', common.mustCall(function() {
    for (let i = 0; i < events.defaultMaxListeners + 1; i++) {
      socket[kReinitializeHandle]();
    }

    socket.destroy();
  }));

  socket.on('close', function() {
    server.close();
  });
}));

It would also be nice to find the root cause of the issue (why socket[kReinitializeHandle]() is called more than 10 times) but I understand it is harder and that is why i pinged the author of the autoSelectFamily feature.

[bnoordhuis]

When implementing it i noticed that TLS interacts with sockets initialization in many place and different ways. So reinitalizing was the only approach I found. Can you help me understand what is improvement you are proposing?

The goal of the reinit hook seems to be to decouple net and tls but the tls module has to call down into the net reinit hook anyway, defeating the point. Tighter coupling likely gives you tighter control.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/54852/

[pimterry]

Something like this should work...

Nice solution @lpinca! Done 👍

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/54854/

[nodejs-github-bot]

Landed in dc04c5e

[silverwind]

Can this be backported to v20?

[MirKml]

Can this be backported to v20?

Yes, I think it's necessary because of npm, which is bundled with node. It emits lots of these warnings event with latest npm versions.

[BCsabaEngine]

Can this be backported to v20?

Yes, I think it's necessary because of npm, which is bundled with node. It emits lots of these warnings event with latest npm versions.

Node20 is LTS now, cannot be stable release eith these error.