events: fix adding abort listener in events.once

Event listeners passed to un/subscribe the abort event are mismatched. This removes the wrapper function in `eventTargetAgnosticAddListener()` and directly passes the given listener to the `EventTarget`. IMO, removing the wrapper seems harmless, and the `AbortSignal` is seemingly the only `EventTarget` passed to this function for now. Fixes: #43337 Refs: #33659 Refs: #34997 Signed-off-by: Daeyeon Jeong [email protected] PR-URL: #43373 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs · Jun 16, 2022 · 0c5092c · 0c5092c
1 parent 517f17b
commit 0c5092c
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 5 deletions.
diff --git a/lib/events.js b/lib/events.js
@@ -972,6 +972,8 @@ async function once(emitter, name, options = kEmptyObject) {
     };
     eventTargetAgnosticAddListener(emitter, name, resolver, { once: true });
     if (name !== 'error' && typeof emitter.once === 'function') {
+      // EventTarget does not have `error` event semantics like Node
+      // EventEmitters, we listen to `error` events only on EventEmitters.
       emitter.once('error', errorListener);
     }
     function abortListener() {
@@ -1011,9 +1013,7 @@ function eventTargetAgnosticAddListener(emitter, name, listener, flags) {
       emitter.on(name, listener);
     }
   } else if (typeof emitter.addEventListener === 'function') {
-    // 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);
+    emitter.addEventListener(name, listener, flags);
   } else {
     throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter);
   }

diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js
@@ -1,5 +1,5 @@
 'use strict';
-// Flags: --no-warnings
+// Flags: --expose-internals --no-warnings
 
 const common = require('../common');
 const { once, EventEmitter } = require('events');
@@ -9,6 +9,7 @@ const {
   fail,
   rejects,
 } = require('assert');
+const { kEvents } = require('internal/event_target');
 
 async function onceAnEvent() {
   const ee = new EventEmitter();
@@ -65,6 +66,32 @@ async function catchesErrors() {
   strictEqual(ee.listenerCount('myevent'), 0);
 }
 
+async function catchesErrorsWithAbortSignal() {
+  const ee = new EventEmitter();
+  const ac = new AbortController();
+  const signal = ac.signal;
+
+  const expected = new Error('boom');
+  let err;
+  process.nextTick(() => {
+    ee.emit('error', expected);
+  });
+
+  try {
+    const promise = once(ee, 'myevent', { signal });
+    strictEqual(ee.listenerCount('error'), 1);
+    strictEqual(signal[kEvents].size, 1);
+
+    await promise;
+  } catch (e) {
+    err = e;
+  }
+  strictEqual(err, expected);
+  strictEqual(ee.listenerCount('error'), 0);
+  strictEqual(ee.listenerCount('myevent'), 0);
+  strictEqual(signal[kEvents].size, 0);
+}
+
 async function stopListeningAfterCatchingError() {
   const ee = new EventEmitter();
 
@@ -165,7 +192,10 @@ async function abortSignalAfterEvent() {
     ee.emit('foo');
     ac.abort();
   });
-  await once(ee, 'foo', { signal: ac.signal });
+  const promise = once(ee, 'foo', { signal: ac.signal });
+  strictEqual(ac.signal[kEvents].size, 1);
+  await promise;
+  strictEqual(ac.signal[kEvents].size, 0);
 }
 
 async function abortSignalRemoveListener() {
@@ -221,6 +251,7 @@ Promise.all([
   onceAnEventWithNullOptions(),
   onceAnEventWithTwoArgs(),
   catchesErrors(),
+  catchesErrorsWithAbortSignal(),
   stopListeningAfterCatchingError(),
   onceError(),
   onceWithEventTarget(),