Skip to content

Commit

Permalink
events: remove the abort listener on iterator completion
Browse files Browse the repository at this point in the history
Given an AbortSignal when passed into the events.on AsyncIterator API
the attached abort listener should always be removed when the iterator
completes.

The abortHandler function is declared within the scope of
the events.on function so cannot be removed by the caller which can lead
to a memory leak. Adding the abort listener using the addEventListener
helper returned by the listenersController helper adds the abort listener
to the array of listeners that will be cleaned up by removeAll in the
closeHandler for the AsyncIterator.

Fixes: nodejs#51010
  • Loading branch information
nbbeeken committed Jan 27, 2024
1 parent af3e2b2 commit 883b575
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
2 changes: 1 addition & 1 deletion lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -1169,7 +1169,7 @@ function on(emitter, event, options = kEmptyObject) {
}
if (signal) {
kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
eventTargetAgnosticAddListener(
addEventListener(
signal,
'abort',
abortListener,
Expand Down
24 changes: 24 additions & 0 deletions test/parallel/test-events-on-async-iterator.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const assert = require('assert');
const { on, EventEmitter } = require('events');
const {
NodeEventTarget,
kEvents
} = require('internal/event_target');

async function basic() {
Expand Down Expand Up @@ -372,6 +373,28 @@ async function abortableOnAfterDone() {
});
}

async function abortListenerRemovedAfterComplete() {
const ee = new EventEmitter();
const ac = new AbortController();

const i = setInterval(() => ee.emit('foo', 'foo'), 1);
try {
// Return case
const endedIterator = on(ee, 'foo', { signal: ac.signal });
assert.ok(ac.signal[kEvents].size > 0);
endedIterator.return();
assert.strictEqual(ac.signal[kEvents].size, 0);

// Throw case
const throwIterator = on(ee, 'foo', { signal: ac.signal });
assert.ok(ac.signal[kEvents].size > 0);
throwIterator.throw(new Error());
assert.strictEqual(ac.signal[kEvents].size, 0);
} finally {
clearInterval(i);
}
}

async function run() {
const funcs = [
basic,
Expand All @@ -391,6 +414,7 @@ async function run() {
eventTargetAbortableOnAfter,
eventTargetAbortableOnAfter2,
abortableOnAfterDone,
abortListenerRemovedAfterComplete,
];

for (const fn of funcs) {
Expand Down

0 comments on commit 883b575

Please sign in to comment.