domains: emit uncaughtException when appropriate

Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when no error
handler is set on the domain within which an error is thrown and an
`'uncaughtException'` event listener is set on the process.

Fixes #3607.

Author: Julien Gilli

lib/domain.js

@@ -96,14 +96,20 @@ Domain.prototype._errorHandler = function errorHandler(er) {
   // that these exceptions are caught, and thus would prevent it from
   // aborting in these cases.
   if (stack.length === 1) {
-    try {
-      // Set the _emittingTopLevelDomainError 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
-      process._emittingTopLevelDomainError = true;
-      caught = emitError();
-    } finally {
-      process._emittingTopLevelDomainError = false;
+    // 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 (self.listeners('error').length > 0) {
+      try {
+        // Set the _emittingTopLevelDomainError 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
+        process._emittingTopLevelDomainError = true;
+        caught = emitError();
+      } finally {
+        process._emittingTopLevelDomainError = false;
+      }
     }
   } else {
     // wrap this in a try/catch so we don't get infinite throwing

test/parallel/test-domain-errors.js

@@ -0,0 +1,203 @@
+'use strict';
+
+/*
+ * The goal of this test is to make sure that errors thrown within domains
+ * are handled correctly. It checks that the process 'uncaughtException' event
+ * is emitted when appropriate, and not emitted when it shouldn't. It also
+ * checks that the proper domain error handlers are called when they should
+ * be called, and not called when they shouldn't.
+ */
+
+const common = require('../common');
+const assert = require('assert');
+const domain = require('domain');
+const child_process = require('child_process');
+
+const uncaughtExceptions = {};
+
+const tests = [];
+
+function test1() {
+  /*
+   * Throwing from an async callback from within a domain that doesn't have
+   * an error handler must result in emitting the process' uncaughtException
+   * event.
+   */
+  const d = domain.create();
+  d.run(function() {
+    setTimeout(function onTimeout() {
+      throw new Error('boom!');
+    });
+  });
+}
+
+tests.push({
+  fn: test1,
+  expectedMessages: ['uncaughtException']
+});
+
+function test2() {
+  /*
+   * Throwing from from within a domain that doesn't have an error handler must
+   * result in emitting the process' uncaughtException event.
+   */
+  const d2 = domain.create();
+  d2.run(function() {
+    throw new Error('boom!');
+  });
+}
+
+tests.push({
+  fn: test2,
+  expectedMessages: ['uncaughtException']
+});
+
+function test3() {
+  /*
+   * This test creates two nested domains: d3 and d4. d4 doesn't register an
+   * error handler, but d3 does. The error is handled by the d3 domain and thus
+   * and 'uncaughtException' event should _not_ be emitted.
+   */
+  const d3 = domain.create();
+  const d4 = domain.create();
+
+  d3.on('error', function onErrorInD3Domain(err) {
+    process.send('errorHandledByDomain');
+  });
+
+  d3.run(function() {
+    d4.run(function() {
+      throw new Error('boom!');
+    });
+  });
+}
+
+tests.push({
+  fn: test3,
+  expectedMessages: ['errorHandledByDomain']
+});
+
+function test4() {
+  /*
+   * This test creates two nested domains: d5 and d6. d6 doesn't register an
+   * error handler. When the timer's callback is called, because async
+   * operations like timer callbacks are bound to the domain that was active
+   * at the time of their creation, and because both d5 and d6 domains have
+   * exited by the time the timer's callback is called, its callback runs with
+   * only d6 on the domains stack. Since d6 doesn't register an error handler,
+   * the process' uncaughtException event should be emitted.
+   */
+  const d5 = domain.create();
+  const d6 = domain.create();
+
+  d5.on('error', function onErrorInD2Domain(err) {
+    process.send('errorHandledByDomain');
+  });
+
+  d5.run(function() {
+    d6.run(function() {
+      setTimeout(function onTimeout() {
+        throw new Error('boom!');
+      });
+    });
+  });
+}
+
+tests.push({
+  fn: test4,
+  expectedMessages: ['uncaughtException']
+});
+
+function test5() {
+  /*
+   * This test creates two nested domains: d7 and d4. d8 _does_ register an
+   * error handler, so throwing within that domain should not emit an uncaught
+   * exception.
+   */
+  const d7 = domain.create();
+  const d8 = domain.create();
+
+  d8.on('error', function onErrorInD3Domain(err) {
+    process.send('errorHandledByDomain');
+  });
+
+  d7.run(function() {
+    d8.run(function() {
+      throw new Error('boom!');
+    });
+  });
+}
+tests.push({
+  fn: test5,
+  expectedMessages: ['errorHandledByDomain']
+});
+
+function test6() {
+  /*
+   * This test creates two nested domains: d9 and d10. d10 _does_ register an
+   * error handler, so throwing within that domain in an async callback should
+   * _not_ emit an uncaught exception.
+   */
+  const d9 = domain.create();
+  const d10 = domain.create();
+
+  d10.on('error', function onErrorInD2Domain(err) {
+    process.send('errorHandledByDomain');
+  });
+
+  d9.run(function() {
+    d10.run(function() {
+      setTimeout(function onTimeout() {
+        throw new Error('boom!');
+      });
+    });
+  });
+}
+
+tests.push({
+  fn: test6,
+  expectedMessages: ['errorHandledByDomain']
+});
+
+if (process.argv[2] === 'child') {
+  const testIndex = process.argv[3];
+  process.on('uncaughtException', function onUncaughtException() {
+    process.send('uncaughtException');
+  });
+
+  tests[testIndex].fn();
+} else {
+  // Run each test's function in a child process. Listen on
+  // messages sent by each child process and compare expected
+  // messages defined for each ttest from the actual received messages.
+  tests.forEach(function doTest(test, testIndex) {
+    const testProcess = child_process.fork(__filename, ['child', testIndex]);
+
+    testProcess.on('message', function onMsg(msg) {
+      if (test.messagesReceived === undefined)
+        test.messagesReceived = [];
+
+      test.messagesReceived.push(msg);
+    });
+
+    testProcess.on('exit', function onExit() {
+      // Make sure that all expected messages were sent from the
+      // child process
+      test.expectedMessages.forEach(function(expectedMessage) {
+        if (test.messagesReceived === undefined ||
+          test.messagesReceived.indexOf(expectedMessage) === -1)
+          assert(false, 'test ' + test.fn.name + ' should have sent message: '
+            + expectedMessage + ' but didn\'t');
+      });
+
+      if (test.messagesReceived) {
+        test.messagesReceived.forEach(function(receivedMessage) {
+          if (test.expectedMessages.indexOf(receivedMessage) === -1) {
+            assert(false, 'test ' + test.fn.name +
+              ' should have sent message: ' + receivedMessage + ' but did');
+          }
+        });
+      }
+    });
+  });
+}