Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
+20 −2
Open

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6e33069
net: do not duplicate `connect` and `finish` listners on `destroySoon`
arturgawlik Oct 28, 2025
9905cfb
unit test for not duplicating event listeners
arturgawlik Oct 28, 2025
afe8307
add messages to test assertions
arturgawlik Oct 28, 2025
565e2c2
use `assert.strictEqual` instead of `assert.equal`
arturgawlik Oct 28, 2025
94edac0
require `common` module
arturgawlik Oct 28, 2025
e5ec2a4
remove third argument on `strictEqual` call
arturgawlik Oct 28, 2025
ae5739e
use `addresses.INVALID_HOST` in test
arturgawlik Oct 28, 2025
a70ef63
`return` from `_final` when `connecting` even if no listeners added
arturgawlik Oct 29, 2025
a63fb37
rework test to better reproduce mentioned issue
arturgawlik Oct 30, 2025
40ef455
revert changes in `_final`
arturgawlik Oct 31, 2025
c2e85a0
make `destroySoon` noop after first call
arturgawlik Nov 1, 2025
3d238a0
remove unwanted brackets
arturgawlik Nov 1, 2025
81ac752
remove unwanted formatting changes
arturgawlik Nov 2, 2025
f703403
Update test/parallel/test-net-socket-not-duplicates-destroy-soon-list…
arturgawlik Nov 2, 2025
7b5a01a
Update test/parallel/test-net-socket-not-duplicates-destroy-soon-list…
arturgawlik Nov 2, 2025
c9a2c5e
use symbol for storing `destroyingOnFinish` flag
arturgawlik Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,8 +529,9 @@ Socket.prototype._unrefTimer = function _unrefTimer() {
// sent out to the other side.
Socket.prototype._final = function(cb) {
// If still connecting - defer handling `_final` until 'connect' will happen
if (this.connecting) {
if (this.connecting && !this._finalizingOnConnect) {
Copy link
Member

@lpinca lpinca Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@arturgawlik arturgawlik Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@lpinca lpinca Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

@arturgawlik arturgawlik Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

debug('_final: not yet connected');
this._finalizingOnConnect = true;
return this.once('connect', () => this._final(cb));
}

Expand Down Expand Up @@ -800,8 +801,10 @@ Socket.prototype.destroySoon = function() {

if (this.writableFinished)
this.destroy();
else
else if (!this._destroyingOnFinish) {
this._destroyingOnFinish = true;
this.once('finish', this.destroy);
}
};


Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-net-socket-not-duplicates-destroy-soon-listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';
require('../common');
const { addresses } = require('../common/internet');

const assert = require('assert');
const net = require('net');

const socket = new net.Socket();
socket.on('error', () => {
// noop
});
socket.connect({ host: addresses.INVALID_HOST, port: 1234 });
socket.destroySoon();
socket.destroySoon();
const finishListenersCount = socket.listeners('finish').length;
const connectListenersCount = socket.listeners('connect').length;
assert.strictEqual(finishListenersCount, 1);
assert.strictEqual(connectListenersCount, 1);
Loading