events.on AsyncIterator API does not remove abort listener from signal in closeHandler

[nbbeeken]

Version

v20.10.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

events

What steps will reproduce the bug?

Hello!
The AbortSignal provided to the events.on API adds an abort listener and does not remove it after the iterator is complete. This causes a warning about a leak if on() is called enough times with the same signal.

Unfortunately, removeEventListener() cannot succeed without a reference to the original function.

Bear with the code sample size, tracking the calls to add and remove listeners required a bit of preamble:

import assert from 'node:assert/strict';
import { EventEmitter, on } from 'node:events';

class SpiedAbort {
  removeListenerCalls = 0;
  addListenerCalls = 0;

  constructor() {
    this.controller = new AbortController();
    const { signal } = this.controller;

    const originalAddEventListener = signal.addEventListener.bind(signal);
    signal.addEventListener = (...args) => {
      this.addListenerCalls += 1;
      return originalAddEventListener(...args);
    };

    const originalRemoveEventListener = signal.removeEventListener.bind(signal);
    signal.removeEventListener = (...args) => {
      this.removeListenerCalls += 1;
      return originalRemoveEventListener(...args);
    };
  }
}

const spiedAbort = new SpiedAbort();
const ee = new EventEmitter();
const iter = on(ee, 'myEvent', { signal: spiedAbort.controller.signal });

iter.return(); // Iterator cleaned up

assert.ok(
  spiedAbort.addListenerCalls >= 1,
  'expected at least one abort listener'
);
assert.equal(
  spiedAbort.removeListenerCalls,
  spiedAbort.addListenerCalls,
  'expected abort listener to be removed as many times as it was added'
);

How often does it reproduce? Is there a required condition?

Every time on() is invoked, a new listener is added, and when the iterator completes it will not be removed.

What is the expected behavior? Why is that the expected behavior?

I would expect the abort listener to be removed when the iterator is complete, regardless of success or failure.

What do you see instead?

When using the same signal across on calls:

import { EventEmitter, on } from 'node:events';
const ee = new EventEmitter();
const controller = new AbortController();
const { signal } = controller;

setInterval(() => ee.emit('myEvent', 1), 1);

for (let i = 0; i < 11; i++) {
  for await (const [data] of on(ee, 'myEvent', { signal })) break;
}

The MaxListenersExceededWarning is printed:

(node:27304) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
    at [kNewListener] (node:internal/event_target:560:17)
    at [kNewListener] (node:internal/abort_controller:241:24)
    at EventTarget.addEventListener (node:internal/event_target:673:23)
    at eventTargetAgnosticAddListener (node:events:1030:13)
    at on (node:events:1159:5)
    at file:///leak.mjs:11:30

Additional information

The referenced closeHandler() can be found here:

node/lib/events.js

Lines 1203 to 1204 in 60ffa9f

	function closeHandler() {
	removeAll();