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

arturgawlik · 2025-10-28T21:21:18Z

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 · 2025-10-28T21:21:23Z

Review requested:

@nodejs/net

codecov · 2025-10-28T23:37:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.54%. Comparing base (fb34515) to head (c9a2c5e).
⚠️ Report is 300 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 · 2025-10-29T21:16:03Z

lib/net.js

 Socket.prototype._final = function(cb) {
  // If still connecting - defer handling `_final` until 'connect' will happen
-  if (this.connecting) {
+  if (this.connecting && !this._finalizingOnConnect) {


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

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

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

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 · 2025-10-30T21:22:33Z

test/parallel/test-net-socket-not-duplicates-destroy-soon-listeners.js

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


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

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.

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.

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

test/parallel/test-net-socket-not-duplicates-destroy-soon-listeners.js

…eners.js Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

mcollina · 2025-11-03T14:18:49Z

lib/net.js

-  else
-    this.once('finish', this.destroy);
+  else {
+    this.destroyingOnFinish = true;


please use a Symbol for this

right 👍 Symbol used ✅

问题： - Socket.io 在高并发连接/断开时抛出 MaxListenersExceededWarning - EventEmitter 默认最多允许 10 个监听器，超过会触发警告 - 参考: nodejs/node#60469 解决方案： 1. 在 afterInit 中设置 Server/Engine/EIO 的最大监听器数量为无限制 (setMaxListeners(0)) 2. 优化 handleDisconnect，确保在断开连接时清理所有事件监听器 3. 实现 OnModuleDestroy 接口，在模块销毁时清理所有 WebSocket 资源修改内容： - 更新 notifications.gateway.ts: * 添加 OnModuleDestroy 接口实现 * 增强 afterInit 方法设置监听器限制 * 优化 handleDisconnect 方法的事件清理 * 实现 onModuleDestroy 方法进行全面的资源清理测试： - npm run build 成功编译 ✅ - 0 个 TypeScript 错误 - 适用于高并发场景（100+ 并发连接）文档： - 添加 docs/WEBSOCKET_MEMORY_LEAK_FIX.md 详细说明修复方案

mcollina

lgtm

nodejs-github-bot · 2025-11-23T09:29:56Z

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

arturgawlik · 2025-12-01T18:36:33Z

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 · 2025-12-04T20:13:12Z

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

nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Oct 28, 2025

arturgawlik force-pushed the do-not-leak-when-destroying-socket-multiple-times branch from 524d5c5 to 8473258 Compare October 28, 2025 21:28

net: do not duplicate connect and finish listners on destroySoon

6e33069

arturgawlik force-pushed the do-not-leak-when-destroying-socket-multiple-times branch from 8473258 to 6e33069 Compare October 28, 2025 21:32

arturgawlik added 2 commits October 28, 2025 22:49

unit test for not duplicating event listeners

9905cfb

add messages to test assertions

afe8307

arturgawlik marked this pull request as ready for review October 28, 2025 22:04

arturgawlik added 4 commits October 28, 2025 23:08

use assert.strictEqual instead of assert.equal

565e2c2

require common module

94edac0

remove third argument on strictEqual call

e5ec2a4

use addresses.INVALID_HOST in test

ae5739e

lpinca reviewed Oct 29, 2025

View reviewed changes

arturgawlik added 2 commits October 29, 2025 23:07

return from _final when connecting even if no listeners added

a70ef63

rework test to better reproduce mentioned issue

a63fb37

lpinca reviewed Oct 30, 2025

View reviewed changes

revert changes in _final

40ef455

arturgawlik marked this pull request as draft November 1, 2025 00:10

make destroySoon noop after first call

c2e85a0

arturgawlik marked this pull request as ready for review November 1, 2025 22:48

arturgawlik added 2 commits November 1, 2025 23:53

remove unwanted brackets

3d238a0

remove unwanted formatting changes

81ac752

lpinca reviewed Nov 2, 2025

View reviewed changes

test/parallel/test-net-socket-not-duplicates-destroy-soon-listeners.js Outdated Show resolved Hide resolved

test/parallel/test-net-socket-not-duplicates-destroy-soon-listeners.js Outdated Show resolved Hide resolved

arturgawlik and others added 2 commits November 2, 2025 23:34

Update test/parallel/test-net-socket-not-duplicates-destroy-soon-list…

f703403

…eners.js Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

Update test/parallel/test-net-socket-not-duplicates-destroy-soon-list…

7b5a01a

…eners.js Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

mcollina reviewed Nov 3, 2025

View reviewed changes

use symbol for storing destroyingOnFinish flag

c9a2c5e

mcollina approved these changes Nov 23, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2025

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

arturgawlik commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 28, 2025

Uh oh!

codecov bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lpinca Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

arturgawlik Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

arturgawlik Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturgawlik Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturgawlik Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

arturgawlik Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 23, 2025

Uh oh!

arturgawlik commented Dec 1, 2025

Uh oh!

nodejs-github-bot commented Dec 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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

arturgawlik commented Oct 28, 2025 •

edited

Loading

codecov bot commented Oct 28, 2025 •

edited

Loading

lpinca Oct 30, 2025 •

edited

Loading

lpinca Oct 31, 2025 •

edited

Loading

arturgawlik Nov 1, 2025 •

edited

Loading