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 88.54%. Comparing base (fb34515) to head (c9a2c5e).
⚠️ Report is 1049 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60469      +/-   ##
==========================================
- Coverage   88.58%   88.54%   -0.05%     
==========================================
  Files         704      704              
  Lines      207826   207886      +60     
  Branches    40054    40050       -4     
==========================================
- Hits       184106   184071      -35     
- Misses      15761    15844      +83     
- Partials     7959     7971      +12     

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

... and 61 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/