Skip to content

Commit

Permalink
lib: disable default memory leak warning for AbortSignal
Browse files Browse the repository at this point in the history
This change sets the default `kMaxEventTargetListeners` property for
`AbortSignal` instances to 0, disabling the check per default, to
enable users to write isomorphic library code.
If desirable, the max event target listeners check can still be
enabled for individual `AbortSignal` instances by calling
`setMaxListeners` on them.

Refs: #54758
PR-URL: #55816
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
  • Loading branch information
phryneas authored Dec 7, 2024
1 parent 9c046ea commit c1ccade
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 11 deletions.
4 changes: 4 additions & 0 deletions doc/api/events.md
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,10 @@ that a "possible EventEmitter memory leak" has been detected. For any single
`EventEmitter`, the `emitter.getMaxListeners()` and `emitter.setMaxListeners()`
methods can be used to temporarily avoid this warning:

`defaultMaxListeners` has no effect on `AbortSignal` instances. While it is
still possible to use [`emitter.setMaxListeners(n)`][] to set a warning limit
for individual `AbortSignal` instances, per default `AbortSignal` instances will not warn.

```mjs
import { EventEmitter } from 'node:events';
const emitter = new EventEmitter();
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/abort_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const {
kResistStopPropagation,
kWeakHandler,
} = require('internal/event_target');
const { kMaxEventTargetListeners } = require('events');
const {
customInspectSymbol,
kEmptyObject,
Expand Down Expand Up @@ -164,6 +165,7 @@ class AbortSignal extends EventTarget {
}
super();

this[kMaxEventTargetListeners] = 0;
const {
aborted = false,
reason = undefined,
Expand Down
34 changes: 23 additions & 11 deletions test/parallel/test-eventtarget-memoryleakwarning.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,22 @@ const { setTimeout } = require('timers/promises');
common.expectWarning({
MaxListenersExceededWarning: [
['Possible EventTarget memory leak detected. 3 foo listeners added to ' +
'EventTarget. MaxListeners is 2. Use events.setMaxListeners() ' +
'EventTarget. MaxListeners is 2. Use events.setMaxListeners() ' +
'to increase limit'],
['Possible EventTarget memory leak detected. 3 foo listeners added to ' +
'[MessagePort [EventTarget]]. ' +
'MaxListeners is 2. ' +
'Use events.setMaxListeners() to increase ' +
'[MessagePort [EventTarget]]. ' +
'MaxListeners is 2. ' +
'Use events.setMaxListeners() to increase ' +
'limit'],
['Possible EventTarget memory leak detected. 3 foo listeners added to ' +
'[MessagePort [EventTarget]]. ' +
'MaxListeners is 2. ' +
'Use events.setMaxListeners() to increase ' +
'[MessagePort [EventTarget]]. ' +
'MaxListeners is 2. ' +
'Use events.setMaxListeners() to increase ' +
'limit'],
['Possible EventTarget memory leak detected. 3 foo listeners added to ' +
'[AbortSignal]. ' +
'MaxListeners is 2. ' +
'Use events.setMaxListeners() to increase ' +
['Possible EventTarget memory leak detected. 2 foo listeners added to ' +
'[AbortSignal]. ' +
'MaxListeners is 1. ' +
'Use events.setMaxListeners() to increase ' +
'limit'],
],
});
Expand Down Expand Up @@ -65,13 +65,25 @@ common.expectWarning({
mc.port1.addEventListener('foo', () => {});
mc.port1.addEventListener('foo', () => {});
mc.port1.addEventListener('foo', () => {});
}

{
// No warning emitted because AbortController ignores `EventEmitter.defaultMaxListeners`
setMaxListeners(2);
const ac = new AbortController();
ac.signal.addEventListener('foo', () => {});
ac.signal.addEventListener('foo', () => {});
ac.signal.addEventListener('foo', () => {});
}

{
// Will still warn as `setMaxListeners` can still manually set a limit
const ac = new AbortController();
setMaxListeners(1, ac.signal);
ac.signal.addEventListener('foo', () => {});
ac.signal.addEventListener('foo', () => {});
}

{
// It works for EventEmitters also
const ee = new EventEmitter();
Expand Down

0 comments on commit c1ccade

Please sign in to comment.