From 6e3901bced0e5ef558f1aeb56e0544964a4bce3f Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Fri, 11 Dec 2015 14:58:56 -0800 Subject: [PATCH] Changes according to latest code review --- test/common.js | 34 ++++++ ...main-no-error-handler-abort-on-uncaught.js | 9 +- ...n-throw-from-uncaught-exception-handler.js | 101 ++++++++++++++++++ .../test-domain-uncaught-exception.js | 12 +-- ...domain-with-abort-on-uncaught-exception.js | 34 +----- 5 files changed, 148 insertions(+), 42 deletions(-) create mode 100644 test/parallel/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js diff --git a/test/common.js b/test/common.js index 572a1dcf982e46..b5e3c2a13b331d 100644 --- a/test/common.js +++ b/test/common.js @@ -449,3 +449,37 @@ exports.fileExists = function(pathname) { exports.fail = function(msg) { assert.fail(null, null, msg); }; + +// 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). + const expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT']; + + // On Windows, v8's base::OS::Abort triggers an access violation, + // which corresponds to exit code 3221225477 (0xC0000005) + if (process.platform === 'win32') + expectedExitCodes = [3221225477]; + + // 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/parallel/test-domain-no-error-handler-abort-on-uncaught.js b/test/parallel/test-domain-no-error-handler-abort-on-uncaught.js index 870afab4ffd27e..1ed7b1ee636e06 100644 --- a/test/parallel/test-domain-no-error-handler-abort-on-uncaught.js +++ b/test/parallel/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) { + const 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/parallel/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js b/test/parallel/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js new file mode 100644 index 00000000000000..499988107dae27 --- /dev/null +++ b/test/parallel/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js @@ -0,0 +1,101 @@ +'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. + +const common = require('../common'); +const assert = require('assert'); +const child_process = require('child_process'); +const domain = require('domain'); + +const uncaughtExceptionHandlerErrMsg = 'boom from uncaughtException handler'; +const domainErrMsg = 'boom from domain'; + +const RAN_UNCAUGHT_EXCEPTION_HANDLER_EXIT_CODE = 42; + +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); + } + })); + + const 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.includes(uncaughtExceptionHandlerErrMsg), + 'stderr output must include proper uncaughtException handler\'s ' + + 'error\'s message'); + assert(!stderr.includes(domainErrMsg), '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/parallel/test-domain-uncaught-exception.js b/test/parallel/test-domain-uncaught-exception.js index f49fd35a5932e2..8792eb1ce5459f 100644 --- a/test/parallel/test-domain-uncaught-exception.js +++ b/test/parallel/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. */ const d3 = domain.create(); const 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) { const 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/parallel/test-domain-with-abort-on-uncaught-exception.js b/test/parallel/test-domain-with-abort-on-uncaught-exception.js index bef952b84354a2..905556a475a75f 100644 --- a/test/parallel/test-domain-with-abort-on-uncaught-exception.js +++ b/test/parallel/test-domain-with-abort-on-uncaught-exception.js @@ -131,38 +131,8 @@ if (process.argv[2] === 'child') { // outside of a try/catch block, the process should not exit gracefully if (!options.useTryCatch && options.throwInDomainErrHandler) { 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. - // - // Depending on the compiler used, node will exit with either - // exit code 132 (SIGILL), 133 (SIGTRAP) or 134 (SIGABRT). - 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). - expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT']; - - // On Windows, v8's base::OS::Abort triggers an access violation, - // which corresponds to exit code 3221225477 (0xC0000005) - if (process.platform === 'win32') - expectedExitCodes = [3221225477]; - - // 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) { - assert.ok(expectedSignals.indexOf(signal) > -1); - } else { - assert.ok(expectedExitCodes.indexOf(exitCode) > -1); - } + 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.