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
arturgawlik wants to merge 16 commits into
base: main
Choose a base branch
from
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 9 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
18 changes: 15 additions & 3 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,14 @@ Socket.prototype._final = function(cb) {
// If still connecting - defer handling `_final` until 'connect' will happen
if (this.connecting) {
debug('_final: not yet connected');
return this.once('connect', () => this._final(cb));
if (!this._finalizingOnConnect) {
this._finalizingOnConnect = true;
this.once('connect', () => {
this._final(cb);
this._finalizingOnConnect = false;
});
}
return;
}

if (!this._handle)
Expand Down Expand Up @@ -800,8 +807,13 @@ Socket.prototype.destroySoon = function() {

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


Expand Down
22 changes: 22 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,22 @@
'use strict';
require('../common');
const { addresses } = require('../common/internet');
Comment thread
arturgawlik marked this conversation as resolved.
Outdated

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

const socket = new net.Socket();
socket.on('error', () => {
// noop
});
const connectOptions = { host: addresses.INVALID_HOST, 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);
Copy link
Copy Markdown
Member

@lpinca lpinca Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
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.

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.

Copy link
Copy Markdown
Member

@lpinca lpinca Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@arturgawlik arturgawlik Nov 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

assert.strictEqual(socket.listeners('finish').length, 1);
assert.strictEqual(socket.listeners('connect').length, 1);
Loading