Skip to content

Commit

Permalink
events: fix adding abort listener in events.once
Browse files Browse the repository at this point in the history
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 daeyeon.dev@gmail.com

PR-URL: #43373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
daeyeon authored and danielleadams committed Jun 16, 2022
1 parent 517f17b commit 0c5092c
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 5 deletions.
6 changes: 3 additions & 3 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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);
}
Expand Down
35 changes: 33 additions & 2 deletions test/parallel/test-events-once.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
'use strict';
// Flags: --no-warnings
// Flags: --expose-internals --no-warnings

const common = require('../common');
const { once, EventEmitter } = require('events');
Expand All @@ -9,6 +9,7 @@ const {
fail,
rejects,
} = require('assert');
const { kEvents } = require('internal/event_target');

async function onceAnEvent() {
const ee = new EventEmitter();
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -221,6 +251,7 @@ Promise.all([
onceAnEventWithNullOptions(),
onceAnEventWithTwoArgs(),
catchesErrors(),
catchesErrorsWithAbortSignal(),
stopListeningAfterCatchingError(),
onceError(),
onceWithEventTarget(),
Expand Down

0 comments on commit 0c5092c

Please sign in to comment.