events.once leaks signal listener

[breavyn]

Version

v18.3.0

Platform

Linux

Subsystem

events

What steps will reproduce the bug?

const { EventEmitter, once } = require("events");

(async () => {
  const controller = new AbortController();
  const ee = new EventEmitter();

  setTimeout(() => {
    controller.abort();
  }, 500);

  while (true) {
    setTimeout(() => ee.emit("test"), 100);
    await once(ee, "test", { signal: controller.signal });

    console.dir(controller.signal);
  }
})();

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

No response

What is the expected behavior?

No response

What do you see instead?

The signal abort listeners do not get removed.

AbortSignal {
  [Symbol(kEvents)]: SafeMap(1) [Map] { 'abort' => { size: 1, next: [Listener] } },
  [Symbol(events.maxEventTargetListeners)]: 10,
  [Symbol(events.maxEventTargetListenersWarned)]: false,
  [Symbol(kAborted)]: false,
  [Symbol(kReason)]: undefined
}
AbortSignal {
  [Symbol(kEvents)]: SafeMap(1) [Map] { 'abort' => { size: 2, next: [Listener] } },
  [Symbol(events.maxEventTargetListeners)]: 10,
  [Symbol(events.maxEventTargetListenersWarned)]: false,
  [Symbol(kAborted)]: false,
  [Symbol(kReason)]: undefined
}
AbortSignal {
  [Symbol(kEvents)]: SafeMap(1) [Map] { 'abort' => { size: 3, next: [Listener] } },
  [Symbol(events.maxEventTargetListeners)]: 10,
  [Symbol(events.maxEventTargetListenersWarned)]: false,
  [Symbol(kAborted)]: false,
  [Symbol(kReason)]: undefined
}
AbortSignal {
  [Symbol(kEvents)]: SafeMap(1) [Map] { 'abort' => { size: 4, next: [Listener] } },
  [Symbol(events.maxEventTargetListeners)]: 10,
  [Symbol(events.maxEventTargetListenersWarned)]: false,
  [Symbol(kAborted)]: false,
  [Symbol(kReason)]: undefined
}

Additional information

When listeners are added to EventTarget's in events.on and events.once, the listener gets wrapped. As AbortSignal is an EventTarget, this means the abort listener can never be removed.

node/lib/events.js

Lines 1011 to 1013 in b631086

	// EventTarget does not have `error` event semantics like Node
	// EventEmitters, we do not listen to `error` events here.
	emitter.addEventListener(name, (arg) => { listener(arg); }, flags);

I confirmed that the listeners do get removed correctly when simply passing the listener in directly, however, I'm unsure if this will have any other consequences hinted at by the comment.

[daeyeon]

IMO, it's possible to directly pass the listener. Sent a PR for this. :-)