From 8a4a4c63699c50e11b9ba72876f6ffd46ba9714a Mon Sep 17 00:00:00 2001
From: Julien Gilli <jgilli@netflix.com>
Date: Tue, 26 Feb 2019 12:01:03 -0800
Subject: [PATCH] domain: allow concurrent user-land impl

Currently, only one domain-lke implementation (the core domain one) can
be used to handle uncaught exceptions or unhandled error events.

This PR aims at making it possible for different domain-like
user-land implementations to be used concurrently (including with the
core domain impl) so that the state of the core domain module (doc
deprecated) does not prevent users of domains from having a
well-maintained domain-like facility.

Ref: https://github.com/nodejs/node/issues/23348
---
 doc/api/errors.md                             | 17 ----
 doc/api/repl.md                               |  9 +-
 lib/domain.js                                 | 87 +++----------------
 lib/events.js                                 | 58 ++++++++++---
 lib/internal/errors.js                        |  9 --
 ...ad-after-set-uncaught-exception-capture.js | 10 ---
 ...t-uncaught-exception-capture-after-load.js | 28 ------
 7 files changed, 59 insertions(+), 159 deletions(-)
 delete mode 100644 test/parallel/test-domain-set-uncaught-exception-capture-after-load.js

diff --git a/doc/api/errors.md b/doc/api/errors.md
index b9d26421497fde..b00976e74f1050 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -818,23 +818,6 @@ A signing `key` was not provided to the [`sign.sign()`][] method.
 
 `c-ares` failed to set the DNS server.
 
-<a id="ERR_DOMAIN_CALLBACK_NOT_AVAILABLE"></a>
-### ERR_DOMAIN_CALLBACK_NOT_AVAILABLE
-
-The `domain` module was not usable since it could not establish the required
-error handling hooks, because
-[`process.setUncaughtExceptionCaptureCallback()`][] had been called at an
-earlier point in time.
-
-<a id="ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE"></a>
-### ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE
-
-[`process.setUncaughtExceptionCaptureCallback()`][] could not be called
-because the `domain` module has been loaded at an earlier point in time.
-
-The stack trace is extended to include the point in time at which the
-`domain` module had been loaded.
-
 <a id="ERR_ENCODING_INVALID_ENCODED_DATA"></a>
 ### ERR_ENCODING_INVALID_ENCODED_DATA
 
diff --git a/doc/api/repl.md b/doc/api/repl.md
index 324b15f3852288..703286843ad86a 100644
--- a/doc/api/repl.md
+++ b/doc/api/repl.md
@@ -142,11 +142,8 @@ global or scoped variable, the input `fs` will be evaluated on-demand as
 The REPL uses the [`domain`][] module to catch all uncaught exceptions for that
 REPL session.
 
-This use of the [`domain`][] module in the REPL has these side effects:
-
-* Uncaught exceptions do not emit the [`'uncaughtException'`][] event.
-* Trying to use [`process.setUncaughtExceptionCaptureCallback()`][] throws
-  an [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`][] error.
+This use of the [`domain`][] module in the REPL has the side effect of making
+uncaught exceptions not emit the [`'uncaughtException'`][] event.
 
 #### Assignment of the `_` (underscore) variable
 <!-- YAML
@@ -636,9 +633,7 @@ For an example of running a REPL instance over [curl(1)][], see:
 
 [`'uncaughtException'`]: process.html#process_event_uncaughtexception
 [`--experimental-repl-await`]: cli.html#cli_experimental_repl_await
-[`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`]: errors.html#errors_err_domain_cannot_set_uncaught_exception_capture
 [`domain`]: domain.html
-[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
 [`readline.InterfaceCompleter`]: readline.html#readline_use_of_the_completer_function
 [`readline.Interface`]: readline.html#readline_class_interface
 [`repl.ReplServer`]: #repl_class_replserver
diff --git a/lib/domain.js b/lib/domain.js
index 031350746cc5b8..4812662a61655b 100644
--- a/lib/domain.js
+++ b/lib/domain.js
@@ -28,17 +28,17 @@
 
 const util = require('util');
 const EventEmitter = require('events');
-const {
-  ERR_DOMAIN_CALLBACK_NOT_AVAILABLE,
-  ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE,
-  ERR_UNHANDLED_ERROR
-} = require('internal/errors').codes;
 const { createHook } = require('async_hooks');
 
 // TODO(addaleax): Use a non-internal solution for this.
 const kWeak = Symbol('kWeak');
 const { WeakReference } = internalBinding('util');
 
+// Communicate with events module, but don't require that
+// module to have to load this one, since this module has
+// a few side effects.
+EventEmitter.usingDomains = true;
+
 // Overwrite process.domain with a getter/setter that will allow for more
 // effective optimizations
 var _domain = [null];
@@ -80,23 +80,7 @@ const asyncHook = createHook({
   }
 });
 
-// When domains are in use, they claim full ownership of the
-// uncaught exception capture callback.
-if (process.hasUncaughtExceptionCaptureCallback()) {
-  throw new ERR_DOMAIN_CALLBACK_NOT_AVAILABLE();
-}
-
-// Get the stack trace at the point where `domain` was required.
-// eslint-disable-next-line no-restricted-syntax
-const domainRequireStack = new Error('require(`domain`) at this point').stack;
-
 const { setUncaughtExceptionCaptureCallback } = process;
-process.setUncaughtExceptionCaptureCallback = function(fn) {
-  const err = new ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE();
-  err.stack = err.stack + '\n' + '-'.repeat(40) + '\n' + domainRequireStack;
-  throw err;
-};
-
 
 let sendMakeCallbackDeprecation = false;
 function emitMakeCallbackDeprecation() {
@@ -123,10 +107,10 @@ function topLevelDomainCallback(cb, ...args) {
   return ret;
 }
 
-// It's possible to enter one domain while already inside
-// another one. The stack is each entered domain.
-const stack = [];
-exports._stack = stack;
+
+// It's possible to enter one domain while already inside another one. The stack
+// is each entered domain.
+const stack = exports._stack = process._domainsStack;
 internalBinding('domain').enable(topLevelDomainCallback);
 
 function updateExceptionCapture() {
@@ -181,6 +165,8 @@ class Domain extends EventEmitter {
   }
 }
 
+Domain[EventEmitter.domainSym] = true;
+
 exports.Domain = Domain;
 
 exports.create = exports.createDomain = function createDomain() {
@@ -307,7 +293,7 @@ Domain.prototype.add = function(ee) {
   // d.add(e);
   // e.add(d);
   // e.emit('error', er); // RangeError, stack overflow!
-  if (this.domain && (ee instanceof Domain)) {
+  if (this.domain && ee[EventEmitter.domainSym]) {
     for (var d = this.domain; d; d = d.domain) {
       if (ee === d) return;
     }
@@ -410,52 +396,3 @@ Domain.prototype.bind = function(cb) {
 
   return runBound;
 };
-
-// Override EventEmitter methods to make it domain-aware.
-EventEmitter.usingDomains = true;
-
-const eventInit = EventEmitter.init;
-EventEmitter.init = function() {
-  this.domain = null;
-  if (exports.active && !(this instanceof exports.Domain)) {
-    this.domain = exports.active;
-  }
-
-  return eventInit.call(this);
-};
-
-const eventEmit = EventEmitter.prototype.emit;
-EventEmitter.prototype.emit = function(...args) {
-  const domain = this.domain;
-
-  const type = args[0];
-  const shouldEmitError = type === 'error' &&
-                          this.listenerCount(type) > 0;
-
-  // 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);
-  }
-
-  if (type === 'error') {
-    const er = args.length > 1 && args[1] ?
-      args[1] : new ERR_UNHANDLED_ERROR();
-
-    if (typeof er === 'object') {
-      er.domainEmitter = this;
-      er.domain = domain;
-      er.domainThrown = false;
-    }
-
-    domain.emit('error', er);
-    return false;
-  }
-
-  domain.enter();
-  const ret = Reflect.apply(eventEmit, this, args);
-  domain.exit();
-
-  return ret;
-};
diff --git a/lib/events.js b/lib/events.js
index 2d7ae8945ff0b2..965399e76ad051 100644
--- a/lib/events.js
+++ b/lib/events.js
@@ -32,7 +32,9 @@ module.exports = EventEmitter;
 EventEmitter.EventEmitter = EventEmitter;
 
 EventEmitter.usingDomains = false;
+EventEmitter.domainSym = Symbol('isDomainLike');
 
+EventEmitter.prototype.domain = undefined;
 EventEmitter.prototype._events = undefined;
 EventEmitter.prototype._eventsCount = 0;
 EventEmitter.prototype._maxListeners = undefined;
@@ -72,6 +74,12 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
 });
 
 EventEmitter.init = function() {
+  this.domain = null;
+  // If there is an active domain, then attach to it, except if the event
+  // emitter being constructed is itself an instance of a domain-like class.
+  if (EventEmitter.usingDomains && !this.constructor[EventEmitter.domainSym]) {
+    this.domain = process.domain;
+  }
 
   if (this._events === undefined ||
       this._events === Object.getPrototypeOf(this)._events) {
@@ -152,12 +160,25 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
   else if (!doError)
     return false;
 
+  const domain = this.domain;
+
   // If there is no 'error' event listener then throw.
   if (doError) {
     let er;
     if (args.length > 0)
       er = args[0];
-    if (er instanceof Error) {
+    if (domain !== null && domain !== undefined) {
+      if (!er) {
+        const errors = lazyErrors();
+        er = new errors.ERR_UNHANDLED_ERROR();
+      }
+      if (typeof er === 'object' && er !== null) {
+        er.domainEmitter = this;
+        er.domain = domain;
+        er.domainThrown = false;
+      }
+      domain.emit('error', er);
+    } else if (er instanceof Error) {
       try {
         const { kExpandStackSymbol } = require('internal/util');
         const capture = {};
@@ -171,21 +192,23 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
       // Note: The comments on the `throw` lines are intentional, they show
       // up in Node's output if this results in an unhandled exception.
       throw er; // Unhandled 'error' event
-    }
+    } else {
+      let stringifiedEr;
+      const { inspect } = require('internal/util/inspect');
+      try {
+        stringifiedEr = inspect(er);
+      } catch {
+        stringifiedEr = er;
+      }
 
-    let stringifiedEr;
-    const { inspect } = require('internal/util/inspect');
-    try {
-      stringifiedEr = inspect(er);
-    } catch {
-      stringifiedEr = er;
+      // At least give some kind of context to the user
+      const errors = lazyErrors();
+      const err = new errors.ERR_UNHANDLED_ERROR(stringifiedEr);
+      err.context = er;
+      throw err; // Unhandled 'error' event
     }
 
-    // At least give some kind of context to the user
-    const errors = lazyErrors();
-    const err = new errors.ERR_UNHANDLED_ERROR(stringifiedEr);
-    err.context = er;
-    throw err; // Unhandled 'error' event
+    return false;
   }
 
   const handler = events[type];
@@ -193,6 +216,12 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
   if (handler === undefined)
     return false;
 
+  let needDomainExit = false;
+  if (domain !== null && domain !== undefined && this !== process) {
+    domain.enter();
+    needDomainExit = true;
+  }
+
   if (typeof handler === 'function') {
     Reflect.apply(handler, this, args);
   } else {
@@ -202,6 +231,9 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
       Reflect.apply(listeners[i], this, args);
   }
 
+  if (needDomainExit)
+    domain.exit();
+
   return true;
 };
 
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index b3b745252a195d..1e68b147428062 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -599,15 +599,6 @@ E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
   'Input buffers must have the same length', RangeError);
 E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]',
   Error);
-E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
-  'A callback was registered through ' +
-     'process.setUncaughtExceptionCaptureCallback(), which is mutually ' +
-     'exclusive with using the `domain` module',
-  Error);
-E('ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE',
-  'The `domain` module is in use, which is mutually exclusive with calling ' +
-     'process.setUncaughtExceptionCaptureCallback()',
-  Error);
 E('ERR_ENCODING_INVALID_ENCODED_DATA',
   'The encoded data was not valid for encoding %s', TypeError);
 E('ERR_ENCODING_NOT_SUPPORTED', 'The "%s" encoding is not supported',
diff --git a/test/parallel/test-domain-load-after-set-uncaught-exception-capture.js b/test/parallel/test-domain-load-after-set-uncaught-exception-capture.js
index 697d4d30806ee7..c941cf94210716 100644
--- a/test/parallel/test-domain-load-after-set-uncaught-exception-capture.js
+++ b/test/parallel/test-domain-load-after-set-uncaught-exception-capture.js
@@ -3,14 +3,4 @@ const common = require('../common');
 
 process.setUncaughtExceptionCaptureCallback(common.mustNotCall());
 
-common.expectsError(
-  () => require('domain'),
-  {
-    code: 'ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
-    type: Error,
-    message: /^A callback was registered.*with using the `domain` module/
-  }
-);
-
-process.setUncaughtExceptionCaptureCallback(null);
 require('domain'); // Should not throw.
diff --git a/test/parallel/test-domain-set-uncaught-exception-capture-after-load.js b/test/parallel/test-domain-set-uncaught-exception-capture-after-load.js
deleted file mode 100644
index e7cbffd00758e2..00000000000000
--- a/test/parallel/test-domain-set-uncaught-exception-capture-after-load.js
+++ /dev/null
@@ -1,28 +0,0 @@
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-
-(function foobar() {
-  require('domain');
-})();
-
-assert.throws(
-  () => process.setUncaughtExceptionCaptureCallback(common.mustNotCall()),
-  (err) => {
-    common.expectsError(
-      {
-        code: 'ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE',
-        type: Error,
-        message: /^The `domain` module is in use, which is mutually/
-      }
-    )(err);
-
-    assert(err.stack.includes('-'.repeat(40)),
-           `expected ${err.stack} to contain dashes`);
-
-    const location = `at foobar (${__filename}:`;
-    assert(err.stack.includes(location),
-           `expected ${err.stack} to contain ${location}`);
-    return true;
-  }
-);