events: remove the abort listener on iterator completion

[nbbeeken]

events: remove abort listener from signal in on

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 addAbortListener helper
returns a disposable that can be used to clean up the listener
when the iterator is exited

Fixes: #51010

[benjamingr]

Where are you actually removing the listener?

[nbbeeken]

Hi, thanks for taking a look, I changed the signal abort listener to be added using addEventListener which comes from the listenersController() helper function. This helper function tracks all add listener calls in this scope and gives us a removeAll helper that is invoked inside the closeHandler that the iterator calls both on error or return.

[nbbeeken]

Hey, @benjamingr anything else I can help with to move this along? Thank you in advance!

[nbbeeken]

Hey @benjamingr sorry for the bump, I appreciate any time you can spend on this with me, thanks in advance. 🙂

I figure it may help to provide another reason for my motivation behind fixing this issue. I attempted working around the leaked listener by recreating a new controller and signal after the iterator was closed. However, it appeared that creating AbortSignals had a surprising performance cost that was preferable to avoid.

Code snippet measuring creating AbortSignals

import perf_hooks from 'node:perf_hooks'
import util from 'node:util'
import os from 'node:os'
import process from 'node:process'

function abortListener() { }
const ITERATIONS = 1000000;

function noop() { }

let shared = null;
function sharedSignal() {
    shared ??= new AbortController().signal;
    shared.addEventListener('abort', abortListener);
    shared.removeEventListener('abort', abortListener);
}

function recreateSignal() {
    const { signal } = new AbortController();
    signal.addEventListener('abort', abortListener);
    signal.removeEventListener('abort', abortListener);
}

const timerFunctions = [noop, sharedSignal, recreateSignal]
    .map(fn => ({ name: fn.name, fn, histogram: perf_hooks.createHistogram() }));

// Warm up
for (const { fn } of timerFunctions) {
    for (let i = 0; i < ITERATIONS; i++) {
        fn();
    }
}

for (const { fn, histogram } of timerFunctions) {
    for (let i = 0; i < ITERATIONS; i++) {
        histogram.recordDelta();
        fn();
        histogram.recordDelta();
    }
}

console.log('ITERATIONS:', ITERATIONS)
console.log('System:', os.platform(), os.arch(), os.cpus().length + ' cores', 'Node.js ' + process.version)
for (const { name, histogram } of timerFunctions) {
    console.log('-'.repeat(20), name, '-'.repeat(20))
    const { mean, stddev } = histogram;
    const median = histogram.percentiles.get(50)
    console.log(util.inspect({ mean, median, stddev }, { breakLength: Infinity, colors: true }))
}

The following numbers are in nanoseconds as measured by histogram.recordDelta(). In ~1ms sharedSignal() can execute ~2,500 times, while in the same time recreateSignal() will execute ~300 times.

And for reference an empty function will execute ~15,800 times.

ITERATIONS: 1000000
System: win32 x64 16 cores Node.js v20.10.0
-------------------- noop --------------------
{ mean: 65.52607426303713, median: 100, stddev: 165.59818354797054 }
-------------------- sharedSignal --------------------
{ mean: 405.4852127426064, median: 500, stddev: 3071.621721322665 }
-------------------- recreateSignal --------------------
{ mean: 3475.6129638064817, median: 5400, stddev: 21840.742869666145 }

[nbbeeken]

I fixed the commit message line length lint in rebase

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 40ef2da