From 0a773dbc55bc5e663c16c2a05bff0d8114b7e9bd Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Thu, 12 Dec 2019 21:08:42 +0100 Subject: [PATCH 1/3] events: allow monitoring error events Installing an error listener has a side effect that emitted errors are considered as handled. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception. There are some workarounds in the wild like monkey patching emit or remit the error if monitoring tool detects that it is the only listener but this is error prone and risky. This PR allows to install a listener to monitor errors with the side effect to consume the error. To avoid conflicts with other events it exports a symbol on EventEmitter which owns this special meaning. Refs: https://github.com/open-telemetry/opentelemetry-js/issues/225 --- doc/api/events.md | 25 +++++++++++++ lib/events.js | 14 ++++++- .../test-event-emitter-error-monitor.js | 37 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-event-emitter-error-monitor.js diff --git a/doc/api/events.md b/doc/api/events.md index 3a558ec233f06b..adcc739ace0647 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -155,6 +155,18 @@ myEmitter.emit('error', new Error('whoops!')); // Prints: whoops! there was an error ``` +It is possible to monitor `'error'` events without consuming the emitted error +by installing a listener using the symbol `errorMonitorSymbol`. + +```js +const myEmitter = new MyEmitter(); +myEmitter.on(errorMonitorSymbol, (err) => { + MyMonitoringTool.log(err); +}); +myEmitter.emit('error', new Error('whoops!')); +// Still throws and crashes Node.js +``` + ## Capture Rejections of Promises > Stability: 1 - captureRejections is experimental. @@ -348,6 +360,19 @@ the event emitter instance, the event’s name and the number of attached listeners, respectively. Its `name` property is set to `'MaxListenersExceededWarning'`. +### EventEmitter.errorMonitorSymbol + + +This symbol shall be used to install a listener only monitoring `'error'` +events. Listeners installed using this symbol are called before the regular +`'error'` listeners are called. + +Installing a listener using this symbol does not change the behavior once a +`'error'` event is emitted, therefore the process will still crash if no +regular `'error'` listener is installed. + ### emitter.addListener(eventName, listener) diff --git a/lib/events.js b/lib/events.js index f765962d21f5a1..be425040ffc11a 100644 --- a/lib/events.js +++ b/lib/events.js @@ -54,7 +54,7 @@ const { } = require('internal/util/inspect'); const kCapture = Symbol('kCapture'); -const kErrorMonitorSymbol = Symbol('events.error-monitor'); +const kErrorMonitor = Symbol('events.errorMonitor'); function EventEmitter(opts) { EventEmitter.init.call(this, opts); @@ -83,8 +83,8 @@ ObjectDefineProperty(EventEmitter, 'captureRejections', { enumerable: true }); -ObjectDefineProperty(EventEmitter, 'errorMonitorSymbol', { - value: kErrorMonitorSymbol, +ObjectDefineProperty(EventEmitter, 'errorMonitor', { + value: kErrorMonitor, writable: false, configurable: true, enumerable: true @@ -264,8 +264,8 @@ EventEmitter.prototype.emit = function emit(type, ...args) { const events = this._events; if (events !== undefined) { - if (doError && events[kErrorMonitorSymbol] !== undefined) - this.emit(kErrorMonitorSymbol, ...args); + if (doError && events[kErrorMonitor] !== undefined) + this.emit(kErrorMonitor, ...args); doError = (doError && events.error === undefined); } else if (!doError) return false; diff --git a/test/parallel/test-event-emitter-error-monitor.js b/test/parallel/test-event-emitter-error-monitor.js index 4eed9e267b15f3..f48f42d87eafc7 100644 --- a/test/parallel/test-event-emitter-error-monitor.js +++ b/test/parallel/test-event-emitter-error-monitor.js @@ -7,7 +7,7 @@ const EE = new EventEmitter(); const theErr = new Error('MyError'); EE.on( - EventEmitter.errorMonitorSymbol, + EventEmitter.errorMonitor, common.mustCall((e) => assert.strictEqual(e, theErr), 3) ); From 793205c1f13767c004eeaab8fb94b0462d09e86b Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Fri, 13 Dec 2019 17:19:05 +0100 Subject: [PATCH 3/3] fix review findings --- doc/api/events.md | 6 +++--- test/parallel/test-event-emitter-error-monitor.js | 13 ++++--------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/doc/api/events.md b/doc/api/events.md index 0c960ad6c503e7..7da2e2318a8e3f 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -160,7 +160,7 @@ by installing a listener using the symbol `errorMonitor`. ```js const myEmitter = new MyEmitter(); -myEmitter.on(errorMonitor, (err) => { +myEmitter.on(EventEmitter.errorMonitor, (err) => { MyMonitoringTool.log(err); }); myEmitter.emit('error', new Error('whoops!')); @@ -365,11 +365,11 @@ Its `name` property is set to `'MaxListenersExceededWarning'`. added: REPLACEME --> -This symbol shall be used to install a listener only monitoring `'error'` +This symbol shall be used to install a listener for only monitoring `'error'` events. Listeners installed using this symbol are called before the regular `'error'` listeners are called. -Installing a listener using this symbol does not change the behavior once a +Installing a listener using this symbol does not change the behavior once an `'error'` event is emitted, therefore the process will still crash if no regular `'error'` listener is installed. diff --git a/test/parallel/test-event-emitter-error-monitor.js b/test/parallel/test-event-emitter-error-monitor.js index f48f42d87eafc7..ba02839a8415e4 100644 --- a/test/parallel/test-event-emitter-error-monitor.js +++ b/test/parallel/test-event-emitter-error-monitor.js @@ -8,7 +8,9 @@ const theErr = new Error('MyError'); EE.on( EventEmitter.errorMonitor, - common.mustCall((e) => assert.strictEqual(e, theErr), 3) + common.mustCall(function onErrorMonitor(e) { + assert.strictEqual(e, theErr); + }, 3) ); // Verify with no error listener @@ -23,14 +25,7 @@ EE.emit('error', theErr); // Verify it works with once process.nextTick(() => EE.emit('error', theErr)); -async function testOnce() { - try { - await EventEmitter.once(EE, 'notTriggered'); - } catch (e) { - assert.strictEqual(e, theErr); - } -} -testOnce(); +assert.rejects(EventEmitter.once(EE, 'notTriggered'), theErr); // Only error events trigger error monitor EE.on('aEvent', common.mustCall());