Changes according to code review

nodejs · misterdjules · Nov 3, 2015 · Dec 11, 2015 · Dec 12, 2015 · b208119f0b17641657916b0d6797a3c45fe8a046
commit b208119f0b17641657916b0d6797a3c45fe8a046
diff --git a/test/common.js b/test/common.js
@@ -226,3 +226,37 @@ exports.busyLoop = function busyLoop(time) {
     ;
   }
 };
+
+// Returns true if the exit code "exitCode" and/or signal name "signal"
+// represent the exit code and/or signal name of a node process that aborted,
+// false otherwise.
+exports.nodeProcessAborted = function nodeProcessAborted(exitCode, signal) {
+  // Depending on the compiler used, node will exit with either
+  // exit code 132 (SIGILL), 133 (SIGTRAP) or 134 (SIGABRT).
+  var expectedExitCodes = [132, 133, 134];
+
+  // On platforms using KSH as the default shell (like SmartOS),
+  // when a process aborts, KSH exits with an exit code that is
+  // greater than 256, and thus the exit code emitted with the 'exit'
+  // event is null and the signal is set to either SIGILL, SIGTRAP,
+  // or SIGABRT (depending on the compiler).
+  var expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT'];
+
+  // On Windows, v8's base::OS::Abort triggers a debug breakpoint,
+  // which makes the process exit with code -2147483645.
+  if (process.platform === 'win32')
+    expectedExitCodes = [-2147483645];
+
+  // When using --abort-on-uncaught-exception, V8 will use
+  // base::OS::Abort to terminate the process.
+  // Depending on the compiler used, the shell or other aspects of
+  // the platform used to build the node binary, this will actually
+  // make V8 exit by aborting or by raising a signal. In any case,
+  // one of them (exit code or signal) needs to be set to one of
+  // the expected exit codes or signals.
+  if (signal !== null) {
+    return expectedSignals.indexOf(signal) > -1;
+  } else {
+    return expectedExitCodes.indexOf(exitCode) > -1;
+  }
+};
diff --git a/test/simple/test-domain-no-error-handler-abort-on-uncaught.js b/test/simple/test-domain-no-error-handler-abort-on-uncaught.js
@@ -180,10 +180,11 @@ if (process.argv[2] === 'child') {
 
     var child = child_process.exec(testCmd);
 
-    child.on('exit', function onExit(code, signal) {
-      assert.ok([132, 133, 134].indexOf(code) !== -1, 'Test at index ' +
-        testIndex + ' should have aborted but instead exited with code ' +
-        code + ' and signal ' + signal);
+    child.on('exit', function onExit(exitCode, signal) {
+      var errMsg = 'Test at index ' + testIndex + ' should have aborted ' +
+          'but instead exited with exit code ' + exitCode + ' and signal ' +
+          signal;
+      assert(common.nodeProcessAborted(exitCode, signal), errMsg);
     });
   });
 }
diff --git a/test/simple/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js b/test/simple/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js
@@ -0,0 +1,102 @@
+'use strict';
+
+// This test makes sure that when throwing an error from a domain, and then
+// handling that error in an uncaughtException handler by throwing an error
+// again, the exit code, signal and error messages are the ones we expect with
+// and without using --abort-on-uncaught-exception.
+
+var common = require('../common');
+var assert = require('assert');
+var child_process = require('child_process');
+var domain = require('domain');
+
+var uncaughtExceptionHandlerErrMsg = 'boom from uncaughtException handler';
+var domainErrMsg = 'boom from domain';
+
+var RAN_UNCAUGHT_EXCEPTION_HANDLER_EXIT_CODE = 42;
+var d;
+
+if (process.argv[2] === 'child') {
+  process.on('uncaughtException', common.mustCall(function onUncaught() {
+    if (process.execArgv.indexOf('--abort-on-uncaught-exception') !== -1) {
+      // When passing --abort-on-uncaught-exception to the child process,
+      // we want to make sure that this handler (the process' uncaughtException
+      // event handler) wasn't called. Unfortunately we can't parse the child
+      // process' output to do that, since on Windows the standard error output
+      // is not properly flushed in V8's Isolate::Throw right before the
+      // process aborts due to an uncaught exception, and thus the error
+      // message representing the error that was thrown cannot be read by the
+      // parent process. So instead of parsing the child process' stdandard
+      // error, the parent process will check that in the case
+      // --abort-on-uncaught-exception was passed, the process did not exit
+      // with exit code RAN_UNCAUGHT_EXCEPTION_HANDLER_EXIT_CODE.
+      process.exit(RAN_UNCAUGHT_EXCEPTION_HANDLER_EXIT_CODE);
+    } else {
+      // On the other hand, when not passing --abort-on-uncaught-exception to
+      // the node process, we want to throw in this event handler to make sure
+      // that the proper error message, exit code and signal are the ones we
+      // expect.
+      throw new Error(uncaughtExceptionHandlerErrMsg);
+    }
+  }));
+
+  d = domain.create();
+  d.run(common.mustCall(function() {
+    throw new Error(domainErrMsg);
+  }));
+} else {
+  runTestWithoutAbortOnUncaughtException();
+  runTestWithAbortOnUncaughtException();
+}
+
+function runTestWithoutAbortOnUncaughtException() {
+  child_process.exec(createTestCmdLine(),
+      function onTestDone(err, stdout, stderr) {
+        // When _not_ passing --abort-on-uncaught-exception, the process'
+        // uncaughtException handler _must_ be called, and thus the error
+        // message must include only the message of the error thrown from the
+        // process' uncaughtException handler.
+        assert(stderr.indexOf(uncaughtExceptionHandlerErrMsg) !== -1,
+            'stderr output must include proper uncaughtException handler\'s ' +
+            'error\'s message');
+        assert(stderr.indexOf(domainErrMsg) === -1, 'stderr output must not ' +
+          'include domain\'s error\'s message');
+
+        assert.notEqual(err.code, 0,
+            'child process should have exited with a non-zero exit code, ' +
+            'but did not');
+      });
+}
+
+function runTestWithAbortOnUncaughtException() {
+  child_process.exec(createTestCmdLine({
+    withAbortOnUncaughtException: true
+  }), function onTestDone(err, stdout, stderr) {
+    assert.notEqual(err.code, RAN_UNCAUGHT_EXCEPTION_HANDLER_EXIT_CODE,
+        'child process should not have run its uncaughtException event ' +
+        'handler');
+    assert(common.nodeProcessAborted(err.code, err.signal),
+        'process should have aborted, but did not');
+  });
+}
+
+function createTestCmdLine(options) {
+  var testCmd = '';
+
+  if (process.platform !== 'win32') {
+    // Do not create core files, as it can take a lot of disk space on
+    // continuous testing and developers' machines
+    testCmd += 'ulimit -c 0 && ';
+  }
+
+  testCmd += process.argv[0];
+
+  if (options && options.withAbortOnUncaughtException) {
+    testCmd += ' ' + '--abort-on-uncaught-exception';
+  }
+
+  testCmd += ' ' + process.argv[1];
+  testCmd += ' ' + 'child';
+
+  return testCmd;
+}
diff --git a/test/simple/test-domain-uncaught-exception.js b/test/simple/test-domain-uncaught-exception.js
@@ -2,7 +2,7 @@
 
 /*
  * The goal of this test is to make sure that errors thrown within domains
- * are handled correctly. It checks that the process 'uncaughtException' event
+ * 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.
@@ -56,7 +56,7 @@ 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.
+   * an 'uncaughtException' event should _not_ be emitted.
    */
   var d3 = domain.create();
   var d4 = domain.create();
@@ -110,7 +110,7 @@ tests.push({
 
 function test5() {
   /*
-   * This test creates two nested domains: d7 and d4. d8 _does_ register an
+   * This test creates two nested domains: d7 and d8. d8 _does_ register an
    * error handler, so throwing within that domain should not emit an uncaught
    * exception.
    */
@@ -169,7 +169,7 @@ if (process.argv[2] === 'child') {
 } 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.
+  // messages defined for each test with the actual received messages.
   tests.forEach(function doTest(test, testIndex) {
     var testProcess = child_process.fork(__filename, ['child', testIndex]);
 
@@ -180,7 +180,7 @@ if (process.argv[2] === 'child') {
       test.messagesReceived.push(msg);
     });
 
-    testProcess.on('exit', function onExit() {
+    testProcess.on('disconnect', common.mustCall(function onExit() {
       // Make sure that all expected messages were sent from the
       // child process
       test.expectedMessages.forEach(function(expectedMessage) {
@@ -200,6 +200,6 @@ if (process.argv[2] === 'child') {
           }
         });
       }
-    })
+    }));
   });
 }
diff --git a/test/simple/test-domain-with-abort-on-uncaught-exception.js b/test/simple/test-domain-with-abort-on-uncaught-exception.js
@@ -21,6 +21,8 @@
 
 var assert = require('assert');
 
+var common = require('../common');
+
 /*
  * The goal of this test is to make sure that:
  *
@@ -131,50 +133,23 @@ if (process.argv[2] === 'child') {
       });
 
       child.on('exit', function onChildExited(exitCode, signal) {
-        // If the top-level domain's error handler does not throw,
-        // the process must exit gracefully, whether or not
-        // --abort-on-uncaught-exception was passed on the command line
-        var expectedExitCode = 0;
-        // On some platforms with KSH being the default shell (like SmartOS),
-        // when a process aborts, KSH exits with an exit code that is greater
-        // than 256, and thus the exit code emitted with the 'exit' event is
-        // null and the signal is set to SIGABRT. For these platforms only,
-        // and when the test is expected to abort, check the actual signal
-        // with the expected signal instead of the exit code.
-        var expectedSignal;
-
-        // When throwing errors from the top-level domain error handler
-        // outside of a try/catch block, the process should not exit gracefully
         if (!options.useTryCatch && options.throwInDomainErrHandler) {
-          expectedExitCode = 7;
           if (cmdLineOption === '--abort-on-uncaught-exception') {
-            // If the top-level domain's error handler throws, and only if
-            // --abort-on-uncaught-exception is passed on the command line,
-            // the process must abort.
-            expectedExitCode = 134;
-
-            // On linux, v8 raises SIGTRAP when aborting because
-            // the "debug break" flag is on by default
-            if (process.platform === 'linux')
-              expectedExitCode = 133;
-
-            if (process.platform === 'sunos') {
-              expectedExitCode = null;
-              expectedSignal = 'SIGABRT';
-            }
-
-            // On Windows, v8's OS::Abort also triggers a debug breakpoint
-            // which makes the process exit with code -2147483645
-            if (process.platform === 'win32') {
-              expectedExitCode = -2147483645;
-            }
+            assert(common.nodeProcessAborted(exitCode, signal),
+              'process should have aborted, but did not');
+          } else {
+            // By default, uncaught exceptions make node exit with an exit
+            // code of 7.
+            assert.equal(exitCode, 7);
+            assert.equal(signal, null);
           }
+        } else {
+          // If the top-level domain's error handler does not throw,
+          // the process must exit gracefully, whether or not
+          // --abort_on_uncaught_exception was passed on the command line
+          assert.equal(exitCode, 0);
+          assert.equal(signal, null);
         }
-
-        if (expectedSignal)
-          assert.equal(signal, expectedSignal)
-
-        assert.equal(exitCode, expectedExitCode);
       });
     }
   }