domain: fix error emit handling

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block. Backport-PR-URL: #18487 PR-URL: #17588 Refs: #17403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Feb 20, 2018 · 35471bcfdffaa9624792a510ad04cbb4c8709193 · 35471bc
1 parent 28edc1d
commit 35471bcfdffaa9624792a510ad04cbb4c8709193
Showing with 40 additions and 17 deletions.

+20 −17 lib/domain.js

+20 −0 test/parallel/test-domain-ee-error-listener.js
diff --git a/lib/domain.js b/lib/domain.js
@@ -399,32 +399,35 @@ EventEmitter.init = function() {
 const eventEmit = EventEmitter.prototype.emit;
 EventEmitter.prototype.emit = function emit(...args) {
   const domain = this.domain;
-  if (domain === null || domain === undefined || this === process) {
-    return Reflect.apply(eventEmit, this, args);
-  }
 
   const type = args[0];
-  // edge case: if there is a domain and an existing non error object is given,
-  // it should not be errorized
-  // see test/parallel/test-event-emitter-no-error-provided-to-error-event.js
-  if (type === 'error' && args.length > 1 && args[1] &&
-    !(args[1] instanceof Error)) {
-    domain.emit('error', args[1]);
-    return false;
-  }
+  const shouldEmitError = type === 'error' &&
+                          this.listenerCount(type) > 0;
 
-  domain.enter();
-  try {
+  // Just call original `emit` if current EE instance has `error`
+  // handler, there's no active domain or this is process
+  if (shouldEmitError || domain === null || domain === undefined ||
+      this === process) {
     return Reflect.apply(eventEmit, this, args);
-  } catch (er) {
-    if (typeof er === 'object' && er !== null) {
+  }
+
+  if (type === 'error') {
+    const er = args.length > 1 && args[1] ?
+      args[1] : new errors.Error('ERR_UNHANDLED_ERROR');
+
+    if (typeof er === 'object') {
       er.domainEmitter = this;
       er.domain = domain;
       er.domainThrown = false;
     }
+
     domain.emit('error', er);
     return false;
-  } finally {
-    domain.exit();
   }
+
+  domain.enter();
+  const ret = Reflect.apply(eventEmit, this, args);
+  domain.exit();
+
+  return ret;
 };
diff --git a/test/parallel/test-domain-ee-error-listener.js b/test/parallel/test-domain-ee-error-listener.js
@@ -0,0 +1,20 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const domain = require('domain').create();
+const EventEmitter = require('events');
+
+domain.on('error', common.mustNotCall());
+
+const ee = new EventEmitter();
+
+const plainObject = { justAn: 'object' };
+ee.once('error', common.mustCall((err) => {
+  assert.deepStrictEqual(err, plainObject);
+}));
+ee.emit('error', plainObject);
+
+const err = new Error('test error');
+ee.once('error', common.expectsError(err));
+ee.emit('error', err);