domain: error handler runs outside of its domain

Before this change, domains' error handlers would run with the
corresponding domain as the active domain. This creates the
possibility for domains' error handlers to call themselves recursively
if an event emitter created in the error handler emits an error, or if
the error handler throws an error.

This change sets the active domain to be the domain's parent (or null
if the domain for which the error handler is called has no parent) to
prevent that from happening.

Fixes: #26086
PR-URL: #26211
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

Author: Julien Gilli

lib/domain.js

@@ -132,7 +132,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);
 
@@ -211,6 +211,13 @@ Domain.prototype._errorHandler = function(er) {
     });
     er.domainThrown = true;
   }
+  // Pop all adjacent duplicates of the currently active 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.
   //
@@ -221,15 +228,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);
@@ -253,10 +260,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];
@@ -486,7 +489,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 common = require('../common');
+const assert = require('assert');
+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);
   }
 }