From 62e24d9239379db21fb4ba4907eb0878be59d9a6 Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sat, 30 May 2020 16:44:35 +0300 Subject: [PATCH 1/5] events: fix EventTarget support Remove support for multiple arguments (which don't actually work for EventTarget). Use our EventTarget implementation rather than a mock. Refactor `once` code in preparation of `on` using shared code for supporting `on` with `EventTarget`s. --- lib/events.js | 61 +++++++++++----------- test/parallel/test-events-once.js | 84 ++++--------------------------- 2 files changed, 41 insertions(+), 104 deletions(-) diff --git a/lib/events.js b/lib/events.js index a5af5ff9309c18..c6b3bc1605fc61 100644 --- a/lib/events.js +++ b/lib/events.js @@ -616,41 +616,18 @@ function unwrapListeners(arr) { function once(emitter, name) { return new Promise((resolve, reject) => { - 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, - (...args) => { resolve(args); }, - { once: true } - ); - return; - } - - const eventListener = (...args) => { - if (errorListener !== undefined) { + const errorListener = (err) => { + emitter.removeListener(name, resolver); + reject(err); + }; + const resolver = (...args) => { + if (typeof emitter.removeListener === 'function') { emitter.removeListener('error', errorListener); } resolve(args); }; - let errorListener; - - // Adding an error listener is not optional because - // if an error is thrown on an event emitter we cannot - // guarantee that the actual event we are waiting will - // be fired. The result could be a silent way to create - // memory or file descriptor leaks, which is something - // we should avoid. - if (name !== 'error') { - errorListener = (err) => { - emitter.removeListener(name, eventListener); - reject(err); - }; - - emitter.once('error', errorListener); - } - - emitter.once(name, eventListener); + eventTargetAgnosticAddListener(emitter, name, resolver, { once: true }); + addErrorHandlerIfEventEmitter(emitter, errorListener, { once: true }); }); } @@ -661,6 +638,28 @@ function createIterResult(value, done) { return { value, done }; } +function addErrorHandlerIfEventEmitter(emitter, handler, flags) { + if (typeof emitter.on === 'function') { + eventTargetAgnosticAddListener(emitter, 'error', handler, flags); + } +} +function eventTargetAgnosticAddListener(emitter, name, listener, flags) { + 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); + return; + } else if (typeof emitter.on === 'function') { + if (flags.once) { + emitter.once(name, listener); + } else { + emitter.on(name, listener); + } + } else { + throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', typeof emitter); + } +} + function on(emitter, event) { const unconsumedEvents = []; const unconsumedPromises = []; diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js index fea143f5877cc7..efc45724028506 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -1,55 +1,10 @@ 'use strict'; +// Flags: --expose-internals const common = require('../common'); const { once, EventEmitter } = require('events'); const { strictEqual, deepStrictEqual } = require('assert'); - -class EventTargetMock { - constructor() { - this.events = {}; - } - - addEventListener = common.mustCall(function(name, listener, options) { - if (!(name in this.events)) { - this.events[name] = { listeners: [], options }; - } - this.events[name].listeners.push(listener); - }); - - removeEventListener = common.mustCall(function(name, callback) { - if (!(name in this.events)) { - return; - } - const event = this.events[name]; - const stack = event.listeners; - - for (let i = 0, l = stack.length; i < l; i++) { - if (stack[i] === callback) { - stack.splice(i, 1); - if (stack.length === 0) { - Reflect.deleteProperty(this.events, name); - } - return; - } - } - }); - - dispatchEvent = function(name, ...arg) { - if (!(name in this.events)) { - return true; - } - const event = this.events[name]; - const stack = event.listeners.slice(); - - for (let i = 0, l = stack.length; i < l; i++) { - stack[i].apply(this, arg); - if (event.options.once) { - this.removeEventListener(name, stack[i]); - } - } - return !name.defaultPrevented; - }; -} +const { EventTarget, Event } = require('internal/event_target'); async function onceAnEvent() { const ee = new EventEmitter(); @@ -104,8 +59,6 @@ async function stopListeningAfterCatchingError() { ee.emit('myevent', 42, 24); }); - process.on('multipleResolves', common.mustNotCall()); - try { await once(ee, 'myevent'); } catch (_e) { @@ -132,38 +85,24 @@ async function onceError() { } async function onceWithEventTarget() { - const et = new EventTargetMock(); - + const et = new EventTarget(); + const event = new Event('myevent'); process.nextTick(() => { - et.dispatchEvent('myevent', 42); + et.dispatchEvent(event); }); const [ value ] = await once(et, 'myevent'); - strictEqual(value, 42); - strictEqual(Reflect.has(et.events, 'myevent'), false); -} - -async function onceWithEventTargetTwoArgs() { - const et = new EventTargetMock(); - - process.nextTick(() => { - et.dispatchEvent('myevent', 42, 24); - }); - - const value = await once(et, 'myevent'); - deepStrictEqual(value, [42, 24]); + strictEqual(value, event); } async function onceWithEventTargetError() { - const et = new EventTargetMock(); - - const expected = new Error('kaboom'); + const et = new EventTarget(); + const error = new Event('error'); process.nextTick(() => { - et.dispatchEvent('error', expected); + et.dispatchEvent(error); }); - const [err] = await once(et, 'error'); - strictEqual(err, expected); - strictEqual(Reflect.has(et.events, 'error'), false); + const [ err ] = await once(et, 'error'); + strictEqual(err, error); } Promise.all([ @@ -173,6 +112,5 @@ Promise.all([ stopListeningAfterCatchingError(), onceError(), onceWithEventTarget(), - onceWithEventTargetTwoArgs(), onceWithEventTargetError(), ]).then(common.mustCall()); From 22d784e467e4610a9634162cf94e978b4efb828e Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sat, 30 May 2020 16:58:24 +0300 Subject: [PATCH 2/5] events: support EventTarget in on Support EventTarget in the `events.on` static method --- lib/events.js | 29 +++++++++---- test/parallel/test-event-on-async-iterator.js | 42 ++++++++++++++++++- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/lib/events.js b/lib/events.js index c6b3bc1605fc61..c6037cefcf2706 100644 --- a/lib/events.js +++ b/lib/events.js @@ -643,20 +643,30 @@ function addErrorHandlerIfEventEmitter(emitter, handler, flags) { eventTargetAgnosticAddListener(emitter, 'error', handler, flags); } } + +function eventTargetAgnosticRemoveListener(emitter, name, listener, flags) { + if (typeof emitter.removeEventListener === 'function') { + emitter.removeEventListener(name, listener, flags); + } else if (typeof emitter.removeListener === 'function') { + emitter.removeListener(name, listener); + } else { + throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter); + } +} + function eventTargetAgnosticAddListener(emitter, name, listener, flags) { 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); - return; } else if (typeof emitter.on === 'function') { - if (flags.once) { + if (flags?.once) { emitter.once(name, listener); } else { emitter.on(name, listener); } } else { - throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', typeof emitter); + throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter); } } @@ -696,8 +706,8 @@ function on(emitter, event) { }, return() { - emitter.removeListener(event, eventHandler); - emitter.removeListener('error', errorHandler); + eventTargetAgnosticRemoveListener(emitter, event, eventHandler); + eventTargetAgnosticRemoveListener(emitter, 'error', errorHandler); finished = true; for (const promise of unconsumedPromises) { @@ -713,8 +723,8 @@ function on(emitter, event) { 'Error', err); } error = err; - emitter.removeListener(event, eventHandler); - emitter.removeListener('error', errorHandler); + eventTargetAgnosticRemoveListener(emitter, event, eventHandler); + eventTargetAgnosticRemoveListener(emitter, 'error', errorHandler); }, [SymbolAsyncIterator]() { @@ -722,8 +732,9 @@ function on(emitter, event) { } }, AsyncIteratorPrototype); - emitter.on(event, eventHandler); - emitter.on('error', errorHandler); + eventTargetAgnosticAddListener(emitter, event, eventHandler); + addErrorHandlerIfEventEmitter(emitter, errorHandler); + return iterator; diff --git a/test/parallel/test-event-on-async-iterator.js b/test/parallel/test-event-on-async-iterator.js index ff5d8cdaf2aea0..9190588bf22665 100644 --- a/test/parallel/test-event-on-async-iterator.js +++ b/test/parallel/test-event-on-async-iterator.js @@ -3,6 +3,11 @@ const common = require('../common'); const assert = require('assert'); const { on, EventEmitter } = require('events'); +const { + EventTarget, + NodeEventTarget, + Event +} = require('internal/event_target'); async function basic() { const ee = new EventEmitter(); @@ -204,6 +209,39 @@ async function iterableThrow() { assert.strictEqual(ee.listenerCount('error'), 0); } +async function eventTarget() { + const et = new EventTarget(); + const tick = () => et.dispatchEvent(new Event('tick')); + const interval = setInterval(tick, 0); + let count = 0; + for await (const [ event ] of on(et, 'tick')) { + count++; + assert.strictEqual(event.type, 'tick'); + if (count >= 5) { + break; + } + } + assert.strictEqual(count, 5); + clearInterval(interval); +} + +async function nodeEventTarget() { + const et = new NodeEventTarget(); + const tick = () => et.dispatchEvent(new Event('tick')); + const interval = setInterval(tick, 0); + let count = 0; + for await (const [ event] of on(et, 'tick')) { + count++; + assert.strictEqual(event.type, 'tick'); + if (count >= 5) { + break; + } + } + assert.strictEqual(count, 5); + clearInterval(interval); +} + + async function run() { const funcs = [ basic, @@ -212,7 +250,9 @@ async function run() { throwInLoop, next, nextError, - iterableThrow + iterableThrow, + eventTarget, + nodeEventTarget ]; for (const fn of funcs) { From 0f2315cafebca8ab64be4408142f337e4d79ea86 Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sat, 30 May 2020 17:19:30 +0300 Subject: [PATCH 3/5] fixup! acorn doesn't like ?. --- lib/events.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/events.js b/lib/events.js index c6037cefcf2706..3b18a964add33f 100644 --- a/lib/events.js +++ b/lib/events.js @@ -660,7 +660,7 @@ function eventTargetAgnosticAddListener(emitter, name, listener, flags) { // EventEmitters, we do not listen to `error` events here. emitter.addEventListener(name, (arg) => { listener(arg); }, flags); } else if (typeof emitter.on === 'function') { - if (flags?.once) { + if (flags && flags.once) { emitter.once(name, listener); } else { emitter.on(name, listener); From a62ff6d5b628d60fca564b2690cea74f407305dd Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sun, 31 May 2020 00:05:34 +0300 Subject: [PATCH 4/5] fixup! fix error listener handler case --- lib/events.js | 8 ++++++-- test/parallel/test-event-on-async-iterator.js | 7 +++++++ test/parallel/test-events-once.js | 4 +++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/events.js b/lib/events.js index 3b18a964add33f..59b8a032e57578 100644 --- a/lib/events.js +++ b/lib/events.js @@ -627,7 +627,9 @@ function once(emitter, name) { resolve(args); }; eventTargetAgnosticAddListener(emitter, name, resolver, { once: true }); - addErrorHandlerIfEventEmitter(emitter, errorListener, { once: true }); + if (name !== 'error') { + addErrorHandlerIfEventEmitter(emitter, errorListener, { once: true }); + } }); } @@ -733,7 +735,9 @@ function on(emitter, event) { }, AsyncIteratorPrototype); eventTargetAgnosticAddListener(emitter, event, eventHandler); - addErrorHandlerIfEventEmitter(emitter, errorHandler); + if (event !== 'error') { + addErrorHandlerIfEventEmitter(emitter, errorHandler); + } return iterator; diff --git a/test/parallel/test-event-on-async-iterator.js b/test/parallel/test-event-on-async-iterator.js index 9190588bf22665..9fc77961dca9d4 100644 --- a/test/parallel/test-event-on-async-iterator.js +++ b/test/parallel/test-event-on-async-iterator.js @@ -225,6 +225,12 @@ async function eventTarget() { clearInterval(interval); } +async function errorListenerCount() { + const et = new EventEmitter(); + on(et, 'foo'); + assert.strictEqual(et.listenerCount('error'), 1); +} + async function nodeEventTarget() { const et = new NodeEventTarget(); const tick = () => et.dispatchEvent(new Event('tick')); @@ -252,6 +258,7 @@ async function run() { nextError, iterableThrow, eventTarget, + errorListenerCount, nodeEventTarget ]; diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js index efc45724028506..7a41722b665bd7 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -78,7 +78,9 @@ async function onceError() { ee.emit('error', expected); }); - const [err] = await once(ee, 'error'); + const promise = once(ee, 'error'); + strictEqual(ee.listenerCount('error'), 1); + const [ err ] = await promise; strictEqual(err, expected); strictEqual(ee.listenerCount('error'), 0); strictEqual(ee.listenerCount('myevent'), 0); From d783fc5acb56536d0b681c0a221fde452b362fa9 Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sun, 31 May 2020 14:44:07 +0300 Subject: [PATCH 5/5] fixup! change order for listeners --- lib/events.js | 16 ++++++++-------- test/parallel/test-events-once.js | 10 +++++++++- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/lib/events.js b/lib/events.js index 59b8a032e57578..065ce9f816129c 100644 --- a/lib/events.js +++ b/lib/events.js @@ -647,26 +647,26 @@ function addErrorHandlerIfEventEmitter(emitter, handler, flags) { } function eventTargetAgnosticRemoveListener(emitter, name, listener, flags) { - if (typeof emitter.removeEventListener === 'function') { - emitter.removeEventListener(name, listener, flags); - } else if (typeof emitter.removeListener === 'function') { + if (typeof emitter.removeListener === 'function') { emitter.removeListener(name, listener); + } else if (typeof emitter.removeEventListener === 'function') { + emitter.removeEventListener(name, listener, flags); } else { throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter); } } function eventTargetAgnosticAddListener(emitter, name, listener, flags) { - 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); - } else if (typeof emitter.on === 'function') { + if (typeof emitter.on === 'function') { if (flags && flags.once) { emitter.once(name, listener); } else { 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); } 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 index 7a41722b665bd7..658a9964be73e1 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -3,7 +3,7 @@ const common = require('../common'); const { once, EventEmitter } = require('events'); -const { strictEqual, deepStrictEqual } = require('assert'); +const { strictEqual, deepStrictEqual, fail } = require('assert'); const { EventTarget, Event } = require('internal/event_target'); async function onceAnEvent() { @@ -107,6 +107,13 @@ async function onceWithEventTargetError() { strictEqual(err, error); } +async function prioritizesEventEmitter() { + const ee = new EventEmitter(); + ee.addEventListener = fail; + ee.removeAllListeners = fail; + process.nextTick(() => ee.emit('foo')); + await once(ee, 'foo'); +} Promise.all([ onceAnEvent(), onceAnEventWithTwoArgs(), @@ -115,4 +122,5 @@ Promise.all([ onceError(), onceWithEventTarget(), onceWithEventTargetError(), + prioritizesEventEmitter(), ]).then(common.mustCall());