domain: error handler runs outside of its domain

lib/domain.js

@@ -125,7 +125,7 @@ function topLevelDomainCallback(cb, ...args) {
 
 // It's possible to enter one domain while already inside
 // another one. The stack is each entered domain.
-const stack = [];
+let stack = [];
 exports._stack = stack;
 internalBinding('domain').enable(topLevelDomainCallback);
 
@@ -199,6 +199,13 @@ Domain.prototype._errorHandler = function(er) {
     er.domain = this;
     er.domainThrown = true;
   }
+  // Pop all adjacent duplicates of the currently acrtive domain from the stack.
+  // This is done to prevent a domain's error handler to run within the context
+  // of itself, and re-entering itself recursively handler as a result of an
+  // exception thrown in its context.
+  while (exports.active === this) {
+    this.exit();
+  }
 
   // The top-level domain-handler is handled separately.
   //
@@ -209,15 +216,15 @@ Domain.prototype._errorHandler = function(er) {
   // process abort. Using try/catch here would always make V8 think
   // that these exceptions are caught, and thus would prevent it from
   // aborting in these cases.
-  if (stack.length === 1) {
+  if (stack.length === 0) {
     // If there's no error handler, do not emit an 'error' event
     // as this would throw an error, make the process exit, and thus
     // prevent the process 'uncaughtException' event from being emitted
     // if a listener is set.
     if (EventEmitter.listenerCount(this, 'error') > 0) {
-      // Clear the uncaughtExceptionCaptureCallback so that we know that, even
-      // if technically the top-level domain is still active, it would
-      // be ok to abort on an uncaught exception at this point
+      // Clear the uncaughtExceptionCaptureCallback so that we know that, since
+      // the top-level domain is not active anymore, it would be ok to abort on
+      // an uncaught exception at this point
       setUncaughtExceptionCaptureCallback(null);
       try {
         caught = this.emit('error', er);
@@ -241,10 +248,6 @@ Domain.prototype._errorHandler = function(er) {
       // The domain error handler threw!  oh no!
       // See if another domain can catch THIS error,
       // or else crash on the original one.
-      // If the user already exited it, then don't double-exit.
-      if (this === exports.active) {
-        stack.pop();
-      }
       updateExceptionCapture();
       if (stack.length) {
         exports.active = process.domain = stack[stack.length - 1];
@@ -449,7 +452,46 @@ EventEmitter.prototype.emit = function(...args) {
       er.domainThrown = false;
     }
 
+    // Remove the current domain (and its duplicates)from the domains stack and
+    // set the active domain to its parent (if any) so that the domain's error
+    // handler doesn't run in its own context. This prevents any event emitter
+    // created or any exception thrown in that error handler from recursively
+    // executing that error handler.
+    const origDomainsStack = stack.slice();
+    const origActiveDomain = process.domain;
+
+    // Travel the domains stack from top to bottom to find the first domain
+    // instance that is not a duplicate of the current active domain.
+    let idx = stack.length - 1;
+    while (idx > -1 && process.domain === stack[idx]) {
+      --idx;
+    }
+
+    // Change the stack to not contain the current active domain, and only the
+    // domains above it on the stack.
+    if (idx < 0) {
+      stack.length = 0;
+    } else {
+      stack.splice(idx + 1);
+    }
+
+    // Change the current active domain
+    if (stack.length > 0) {
+      exports.active = process.domain = stack[stack.length - 1];
+    } else {
+      exports.active = process.domain = null;
+    }
+
+    updateExceptionCapture();
+
     domain.emit('error', er);
+
+    // Now that the domain's error handler has completed, restore the domains
+    // stack and the active domain to their original values.
+    exports._stack = stack = origDomainsStack;
+    exports.active = process.domain = origActiveDomain;
+    updateExceptionCapture();
+
     return false;
   }
 

test/parallel/test-domain-emit-error-handler-stack.js

@@ -0,0 +1,161 @@
+'use strict';
+
+const assert = require('assert');
+const common = require('../common');
+const domain = require('domain');
+const EventEmitter = require('events');
+
+/*
+ * Make sure that the domains stack and the active domain is setup properly when
+ * a domain's error handler is called due to an error event being emitted.
+ * More specifically, we want to test that:
+ * - the active domain in the domain's error handler is *not* that domain, *but*
+ *   the active domain is a any direct parent domain at the time the error was
+ *   emitted.
+ * - the domains stack in the domain's error handler does *not* include that
+ *   domain, *but* it includes all parents of that domain when the error was
+ *   emitted.
+ */
+const d1 = domain.create();
+const d2 = domain.create();
+const d3 = domain.create();
+
+function checkExpectedDomains(err) {
+  // First, check that domains stack and active domain is as expected when the
+  // event handler is called synchronously via ee.emit('error').
+  if (domain._stack.length !== err.expectedStackLength) {
+    console.error('expected domains stack length of %d, but instead is %d',
+                  err.expectedStackLength, domain._stack.length);
+    process.exit(1);
+  }
+
+  if (process.domain !== err.expectedActiveDomain) {
+    console.error('expected active domain to be %j, but instead is %j',
+                  err.expectedActiveDomain, process.domain);
+    process.exit(1);
+  }
+
+  // Then make sure that the domains stack and active domain is setup as
+  // expected when executing a callback scheduled via nextTick from the error
+  // handler.
+  process.nextTick(() => {
+    const expectedStackLengthInNextTickCb =
+            err.expectedStackLength > 0 ? 1 : 0;
+    if (domain._stack.length !== expectedStackLengthInNextTickCb) {
+      console.error('expected stack length in nextTick cb to be %d, ' +
+                'but instead is %d', expectedStackLengthInNextTickCb,
+                    domain._stack.length);
+      process.exit(1);
+    }
+
+    const expectedActiveDomainInNextTickCb =
+            expectedStackLengthInNextTickCb === 0 ? undefined :
+              err.expectedActiveDomain;
+    if (process.domain !== expectedActiveDomainInNextTickCb) {
+      console.error('expected active domain in nextTick cb to be %j, ' +
+                'but instead is %j', expectedActiveDomainInNextTickCb,
+                    process.domain);
+      process.exit(1);
+    }
+  });
+}
+
+d1.on('error', common.mustCall((err) => {
+  checkExpectedDomains(err);
+}, 2));
+
+d2.on('error', common.mustCall((err) => {
+  checkExpectedDomains(err);
+}, 2));
+
+d3.on('error', common.mustCall((err) => {
+  checkExpectedDomains(err);
+}, 1));
+
+d1.run(() => {
+  const ee = new EventEmitter();
+  assert.strictEqual(process.domain, d1);
+  assert.strictEqual(domain._stack.length, 1);
+
+  const err = new Error('oops');
+  err.expectedStackLength = 0;
+  err.expectedActiveDomain = null;
+  ee.emit('error', err);
+
+  assert.strictEqual(process.domain, d1);
+  assert.strictEqual(domain._stack.length, 1);
+});
+
+d1.run(() => {
+  d1.run(() => {
+    const ee = new EventEmitter();
+
+    assert.strictEqual(process.domain, d1);
+    assert.strictEqual(domain._stack.length, 2);
+
+    const err = new Error('oops');
+    err.expectedStackLength = 0;
+    err.expectedActiveDomain = null;
+    ee.emit('error', err);
+
+    assert.strictEqual(process.domain, d1);
+    assert.strictEqual(domain._stack.length, 2);
+  });
+});
+
+d1.run(() => {
+  d2.run(() => {
+    const ee = new EventEmitter();
+
+    assert.strictEqual(process.domain, d2);
+    assert.strictEqual(domain._stack.length, 2);
+
+    const err = new Error('oops');
+    err.expectedStackLength = 1;
+    err.expectedActiveDomain = d1;
+    ee.emit('error', err);
+
+    assert.strictEqual(process.domain, d2);
+    assert.strictEqual(domain._stack.length, 2);
+  });
+});
+
+d1.run(() => {
+  d2.run(() => {
+    d2.run(() => {
+      const ee = new EventEmitter();
+
+      assert.strictEqual(process.domain, d2);
+      assert.strictEqual(domain._stack.length, 3);
+
+      const err = new Error('oops');
+      err.expectedStackLength = 1;
+      err.expectedActiveDomain = d1;
+      ee.emit('error', err);
+
+      assert.strictEqual(process.domain, d2);
+      assert.strictEqual(domain._stack.length, 3);
+    });
+  });
+});
+
+d3.run(() => {
+  d1.run(() => {
+    d3.run(() => {
+      d3.run(() => {
+        const ee = new EventEmitter();
+
+        assert.strictEqual(process.domain, d3);
+        assert.strictEqual(domain._stack.length, 4);
+
+        const err = new Error('oops');
+        err.expectedStackLength = 2;
+        err.expectedActiveDomain = d1;
+        ee.emit('error', err);
+
+        assert.strictEqual(process.domain, d3);
+        assert.strictEqual(domain._stack.length, 4);
+      });
+    });
+  });
+});

test/parallel/test-domain-thrown-error-handler-stack.js

@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common');
+const domain = require('domain');
+
+/*
+ * Make sure that when an erorr is thrown from a nested domain, its error
+ * handler runs outside of that domain, but within the context of any parent
+ * domain.
+ */
+
+const d = domain.create();
+const d2 = domain.create();
+
+d2.on('error', common.mustCall((err) => {
+  if (domain._stack.length !== 1) {
+    console.error('domains stack length should be 1 but is %d',
+                  domain._stack.length);
+    process.exit(1);
+  }
+
+  if (process.domain !== d) {
+    console.error('active domain should be %j but is %j', d, process.domain);
+    process.exit(1);
+  }
+
+  process.nextTick(() => {
+    if (domain._stack.length !== 1) {
+      console.error('domains stack length should be 1 but is %d',
+                    domain._stack.length);
+      process.exit(1);
+    }
+
+    if (process.domain !== d) {
+      console.error('active domain should be %j but is %j', d,
+                    process.domain);
+      process.exit(1);
+    }
+  });
+}));
+
+d.run(() => {
+  d2.run(() => {
+    throw new Error('oops');
+  });
+});

test/parallel/test-domain-top-level-error-handler-clears-stack.js

@@ -10,19 +10,18 @@ const domain = require('domain');
 const d = domain.create();
 
 d.on('error', common.mustCall(() => {
+  // Scheduling a callback with process.nextTick _could_ enter a _new_ domain,
+  // but domain's error handlers are called outside of their domain's context.
+  // So there should _no_ domain on the domains stack if the domains stack was
+  // cleared properly when the domain error handler was called.
   process.nextTick(() => {
-    // Scheduling a callback with process.nextTick will enter a _new_ domain,
-    // and the callback will be called after the domain that handled the error
-    // was exited. So there should be only one domain on the domains stack if
-    // the domains stack was cleared properly when the domain error handler
-    // returned.
-    if (domain._stack.length !== 1) {
+    if (domain._stack.length !== 0) {
       // Do not use assert to perform this test: this callback runs in a
       // different callstack as the original process._fatalException that
       // handled the original error, thus throwing here would trigger another
       // call to process._fatalException, and so on recursively and
       // indefinitely.
-      console.error('domains stack length should be 1, but instead is:',
+      console.error('domains stack length should be 0, but instead is:',
                     domain._stack.length);
       process.exit(1);
     }

test/parallel/test-timers-reset-process-domain-on-throw.js

@@ -26,9 +26,10 @@ function err() {
   }
 
   function handleDomainError(e) {
-    // In the domain's error handler, the current active domain should be the
-    // domain within which the error was thrown.
-    assert.strictEqual(process.domain, d);
+    assert.strictEqual(e.domain, d);
+    // Domains' error handlers are called outside of their domain's context, so
+    // we're not expecting any active domain here.
+    assert.strictEqual(process.domain, undefined);
   }
 }