net: do not add multiple connect and finish listeners when multiple Socket.destroySoon's were scheduled

[arturgawlik]

Fixes #60456 by adding new finish event listener after socket.destroySoon call only if they were not added previously on given socket.

[nodejs-github-bot]

Review requested:

• @nodejs/net

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.11%. Comparing base (f170888) to head (58a7f8d).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60469      +/-   ##
==========================================
- Coverage   90.13%   90.11%   -0.03%     
==========================================
  Files         751      751              
  Lines      253629   253637       +8     
  Branches    47792    47794       +2     
==========================================
- Hits       228615   228566      -49     
- Misses      16269    16325      +56     
- Partials     8745     8746       +1     

Files with missing lines	Coverage Δ
lib/net.js	94.57% <100.00%> (+0.01%)	⬆️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lpinca]

I don't think this guard is need, wriable._final() is only called once.

[arturgawlik]

Sorry, maybe I'm missing something here 🤔 but from what I have been debugged it seems that Socket._final() is called for every socket.destroySoon() call in described scenario in the linked issue #60456

P.S. I've updated implementation to return from this function even if _finalizingOnConnect is already true (as it was previously).

[lpinca]

const net = require('net');

const socket = net.createConnection({
  host: 'example.com',
  port: 80,
  lookup() {}
});

socket.destroySoon();
socket.destroySoon();
socket.destroySoon();
socket.destroySoon();
socket.destroySoon();

console.log(socket.listenerCount('connect')); // Prints 1

[arturgawlik]

You are right. My test didn't reproduce mentioned issue well. I reworked it to do it better. So now it looks like this:

const net = require('net');

const socket = new net.Socket();
socket.on('error', () => {
  // noop
});
const connectOptions = { host: "example.com", port: 1234 };

socket.connect(connectOptions);
socket.destroySoon(); // Adds "connect" and "finish" event listeners when socket has "writable" state
socket.destroy(); // Makes imideditly socket again "writable"

socket.connect(connectOptions);
socket.destroySoon(); // Should not duplicate "connect" and "finish" event listeners

console.log(socket.listeners('finish').length); // Prints 2
console.log(socket.listeners('connect').length); // Prints also 2

and with it we can observe that second call to socket.destroySoon() adds additional event listeners for finish and connect events, which could lead to memory leak.

[lpinca]

This is what actually makes the socket writable again as it calls socket._undestroy(). It is not recommended to call socket.connect() on a previously destroyed socket as its state might not be well defined. The issue you are describing can also happen in other cases with this usage pattern, for example:

const net = require('net');

const options = {
  host: 'example.com',
  port: 80,
  lookup() {}
};

const socket = new net.Socket();

socket.connect(options);
socket.read();
socket.destroy();

socket.connect(options);
socket.read();
socket.destroy();

socket.connect(options);
socket.read();
socket.destroy();

console.log(socket.listenerCount('connect')); // Prints 3

[arturgawlik]

Thank You for Your review! But do You think that mentioned issue should be even addressed? If yes I will analyze it deeper to provide better test that is reproducing this issue.

[lpinca]

I think we should fix the issue where multiple 'finish' events are added if socket.destroySoon() is called multiple times on the same socket. A possible fix is to make socket.destroySoon() a noop after the first call.

[arturgawlik]

I've removed changes for connect event listener, and leaved for finish (in destroySoon). I didn't make socket.destroySoon() completely noop after first call, because I still want this function to work back as it should when finish event eventually is emitted, and calling socket.destroySoon() potentially becomes back legitimate thing. But please let me know if You meant something else by writing "a noop after the first call".

[mcollina]

please use a Symbol for this

[arturgawlik]

right 👍 Symbol used ✅

[mcollina]

lgtm

[nodejs-github-bot]

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

[arturgawlik]

Hi :) I suppose that tests do not pass because of flakiness :/ May I please to add request-ci label by someone with such privilege?

[nodejs-github-bot]

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

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

[github-actions]

Failed to start CI

- Validating Jenkins credentials
✔  Jenkins credentials valid
- Querying data for job/node-test-pull-request/70389/
[SyntaxError: Unexpected token '<', ..."    
  https://github.com/nodejs/node/actions/runs/30332820681

[nodejs-github-bot]

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

[nodejs-github-bot]

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