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] 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
+<!-- YAML
+added: REPLACEME
+-->
+
+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)
 <!-- YAML
 added: v0.1.26
diff --git a/lib/events.js b/lib/events.js
index 13ecbd9ea87433..f765962d21f5a1 100644
--- a/lib/events.js
+++ b/lib/events.js
@@ -54,6 +54,7 @@ const {
 } = require('internal/util/inspect');
 
 const kCapture = Symbol('kCapture');
+const kErrorMonitorSymbol = Symbol('events.error-monitor');
 
 function EventEmitter(opts) {
   EventEmitter.init.call(this, opts);
@@ -82,6 +83,13 @@ ObjectDefineProperty(EventEmitter, 'captureRejections', {
   enumerable: true
 });
 
+ObjectDefineProperty(EventEmitter, 'errorMonitorSymbol', {
+  value: kErrorMonitorSymbol,
+  writable: false,
+  configurable: true,
+  enumerable: true
+});
+
 // The default for captureRejections is false
 ObjectDefineProperty(EventEmitter.prototype, kCapture, {
   value: false,
@@ -255,9 +263,11 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
   let doError = (type === 'error');
 
   const events = this._events;
-  if (events !== undefined)
+  if (events !== undefined) {
+    if (doError && events[kErrorMonitorSymbol] !== undefined)
+      this.emit(kErrorMonitorSymbol, ...args);
     doError = (doError && events.error === undefined);
-  else if (!doError)
+  } else if (!doError)
     return false;
 
   // If there is no 'error' event listener then throw.
diff --git a/test/parallel/test-event-emitter-error-monitor.js b/test/parallel/test-event-emitter-error-monitor.js
new file mode 100644
index 00000000000000..4eed9e267b15f3
--- /dev/null
+++ b/test/parallel/test-event-emitter-error-monitor.js
@@ -0,0 +1,37 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const EventEmitter = require('events');
+
+const EE = new EventEmitter();
+const theErr = new Error('MyError');
+
+EE.on(
+  EventEmitter.errorMonitorSymbol,
+  common.mustCall((e) => assert.strictEqual(e, theErr), 3)
+);
+
+// Verify with no error listener
+common.expectsError(
+  () => EE.emit('error', theErr), theErr
+);
+
+// Verify with error listener
+EE.once('error', common.mustCall((e) => assert.strictEqual(e, theErr)));
+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();
+
+// Only error events trigger error monitor
+EE.on('aEvent', common.mustCall());
+EE.emit('aEvent');