diff --git a/lib/domain.js b/lib/domain.js index 4ee4d71b196c07..fe130cc9015534 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -71,39 +71,54 @@ Domain.prototype._errorHandler = function errorHandler(er) { er.domain = this; er.domainThrown = true; } - // wrap this in a try/catch so we don't get infinite throwing - try { - // One of three things will happen here. - // - // 1. There is a handler, caught = true - // 2. There is no handler, caught = false - // 3. It throws, caught = false - // - // If caught is false after this, then there's no need to exit() - // the domain, because we're going to crash the process anyway. - caught = this.emit('error', er); - - // Exit all domains on the stack. Uncaught exceptions end the - // current tick and no domains should be left on the stack - // between ticks. - stack.length = 0; - exports.active = process.domain = null; - } catch (er2) { - // 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(); + + if (stack.length === 1) { + try { + this.emittingTopLevelError = true; + + caught = this.emit('error', er); + + stack.length = 0; + exports.active = process.domain = null; + } finally { + this.emittingTopLevelError = false; } - if (stack.length) { - exports.active = process.domain = stack[stack.length - 1]; - caught = process._fatalException(er2); - } else { - caught = false; + } else { + // wrap this in a try/catch so we don't get infinite throwing + try { + // One of three things will happen here. + // + // 1. There is a handler, caught = true + // 2. There is no handler, caught = false + // 3. It throws, caught = false + // + // If caught is false after this, then there's no need to exit() + // the domain, because we're going to crash the process anyway. + caught = this.emit('error', er); + + // Exit all domains on the stack. Uncaught exceptions end the + // current tick and no domains should be left on the stack + // between ticks. + stack.length = 0; + exports.active = process.domain = null; + } catch (er2) { + // 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(); + } + if (stack.length) { + exports.active = process.domain = stack[stack.length - 1]; + caught = process._fatalException(er2); + } else { + caught = false; + } + return caught; } - return caught; } + return caught; }; @@ -176,20 +191,29 @@ Domain.prototype.run = function(fn) { if (this._disposed) return; - var ret; + var ret, args; this.enter(); if (arguments.length >= 2) { var len = arguments.length; - var args = new Array(len - 1); + args = new Array(len - 1); for (var i = 1; i < len; i++) args[i - 1] = arguments[i]; + } + // Wrap this in a try/catch block so that + // + // domain.run(function() { throw new Error("foo"); }) + // + // actually emits an error event on the domain. + try { ret = fn.apply(this, args); - } else { - ret = fn.call(this); + } catch (err) { + process._forceTickDone(); + process._fatalException(err); } + this.exit(); return ret; diff --git a/lib/repl.js b/lib/repl.js index 81afd27a4dbd00..923c1d02383ddb 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -159,20 +159,10 @@ function REPLServer(prompt, regExMatcher.test(savedRegExMatches.join(sep)); if (!err) { - try { - if (self.useGlobal) { - result = script.runInThisContext({ displayErrors: false }); - } else { - result = script.runInContext(context, { displayErrors: false }); - } - } catch (e) { - err = e; - if (err && process.domain) { - debug('not recoverable, send to domain'); - process.domain.emit('error', err); - process.domain.exit(); - return; - } + if (self.useGlobal) { + result = script.runInThisContext({ displayErrors: false }); + } else { + result = script.runInContext(context, { displayErrors: false }); } } diff --git a/lib/timers.js b/lib/timers.js index aa9f316253e692..3e2f57e263693d 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -81,24 +81,30 @@ function listOnTimeout() { if (domain && domain._disposed) continue; - try { - if (domain) - domain.enter(); - threw = true; - first._called = true; - first._onTimeout(); - if (domain) - domain.exit(); - threw = false; - } finally { - if (threw) { - // We need to continue processing after domain error handling - // is complete, but not by using whatever domain was left over - // when the timeout threw its exception. - var oldDomain = process.domain; - process.domain = null; - process.nextTick(listOnTimeoutNT, list); - process.domain = oldDomain; + // If there's no active domain or we're in the + // error handler for the domain at the top of the stack, + // don't use try/catch to allow --abort-on-uncaught-exception + // to abort if an error is thrown. + // Otherwise, use try/catch to get the chance to emit an + // error on the current domain + if (!domain || domain.emittingTopLevelError) { + try { + fireTimer(first); + threw = false; + } finally { + if (threw) { + postFireTimer(); + } + } + } else { + try { + fireTimer(first); + threw = false; + } catch (err) { + process._fatalException(err); + if (threw) { + postFireTimer(); + } } } } @@ -108,6 +114,26 @@ function listOnTimeout() { assert(L.isEmpty(list)); list.close(); delete lists[msecs]; + + function fireTimer(timer) { + if (domain) + domain.enter(); + threw = true; + first._called = true; + first._onTimeout(); + if (domain) + domain.exit(); + } + + function postFireTimer() { + // We need to continue processing after domain error handling + // is complete, but not by using whatever domain was left over + // when the timeout threw its exception. + var oldDomain = process.domain; + process.domain = null; + process.nextTick(listOnTimeoutNT, list); + process.domain = oldDomain; + } } @@ -370,20 +396,28 @@ function processImmediate() { domain.enter(); var threw = true; - try { - immediate._onImmediate(); - threw = false; - } finally { - if (threw) { - if (!L.isEmpty(queue)) { - // Handle any remaining on next tick, assuming we're still - // alive to do so. - while (!L.isEmpty(immediateQueue)) { - L.append(queue, L.shift(immediateQueue)); - } - immediateQueue = queue; - process.nextTick(processImmediate); - } + + // If there's no active domain or we're in the + // error handler for the domain at the top of the stack, + // don't use try/catch to allow --abort-on-uncaught-exception + // to abort if an error is thrown. + // Otherwise, use try/catch to get the chance to emit an + // error on the current domain + if (!domain || domain.emittingTopLevelError) { + try { + immediate._onImmediate(); + threw = false; + } finally { + postImmediate(threw); + } + } else { + try { + immediate._onImmediate(); + threw = false; + } catch (err) { + process._fatalException(err); + + postImmediate(threw); } } @@ -397,6 +431,20 @@ function processImmediate() { if (L.isEmpty(immediateQueue)) { process._needImmediateCallback = false; } + + function postImmediate(threw) { + if (threw) { + if (!L.isEmpty(queue)) { + // Handle any remaining on next tick, assuming we're still + // alive to do so. + while (!L.isEmpty(immediateQueue)) { + L.append(queue, L.shift(immediateQueue)); + } + immediateQueue = queue; + process.nextTick(processImmediate); + } + } + } } diff --git a/src/node.cc b/src/node.cc index 1e63009e3bba1e..3975364295fc7b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2180,11 +2180,7 @@ void FatalException(Isolate* isolate, if (false == caught->BooleanValue()) { ReportException(env, error, message); - if (abort_on_uncaught_exception) { - ABORT(); - } else { - exit(1); - } + exit(1); } } @@ -3201,6 +3197,10 @@ static void ParseArgs(int* argc, } else if (strcmp(arg, "--abort-on-uncaught-exception") == 0 || strcmp(arg, "--abort_on_uncaught_exception") == 0) { abort_on_uncaught_exception = true; + // Pass this option to V8 anyway, because V8 uses it to determine + // if it needs to abort on an uncaught exception + new_v8_argv[new_v8_argc] = arg; + new_v8_argc += 1; } else if (strcmp(arg, "--v8-options") == 0) { new_v8_argv[new_v8_argc] = "--help"; new_v8_argc += 1; diff --git a/src/node.js b/src/node.js index 001a47657f816a..cf4754bd7e7126 100644 --- a/src/node.js +++ b/src/node.js @@ -283,6 +283,10 @@ _runMicrotasks = _runMicrotasks.runMicrotasks; + process._forceTickDone = function _forceTickDone() { + process._tickDoneForced = true; + }; + function tickDone() { if (tickInfo[kLength] !== 0) { if (tickInfo[kLength] <= tickInfo[kIndex]) { @@ -358,38 +362,59 @@ } while (tickInfo[kLength] !== 0); } + function performNTCallback(args, callback) { + // Using separate callback execution functions helps to limit the + // scope of DEOPTs caused by using try blocks and allows direct + // callback invocation with small numbers of arguments to avoid the + // performance hit associated with using `fn.apply()` + if (args === undefined) { + doNTCallback0(callback); + } else { + switch (args.length) { + case 1: + doNTCallback1(callback, args[0]); + break; + case 2: + doNTCallback2(callback, args[0], args[1]); + break; + case 3: + doNTCallback3(callback, args[0], args[1], args[2]); + break; + default: + doNTCallbackMany(callback, args); + } + } + } + function _tickDomainCallback() { var callback, domain, args, tock; do { - while (tickInfo[kIndex] < tickInfo[kLength]) { + while (!process._tickDoneForced && + tickInfo[kIndex] < tickInfo[kLength]) { tock = nextTickQueue[tickInfo[kIndex]++]; callback = tock.callback; domain = tock.domain; args = tock.args; if (domain) domain.enter(); - // Using separate callback execution functions helps to limit the - // scope of DEOPTs caused by using try blocks and allows direct - // callback invocation with small numbers of arguments to avoid the - // performance hit associated with using `fn.apply()` - if (args === undefined) { - doNTCallback0(callback); + + // If there's no active domain or we're in the + // error handler for the domain at the top of the stack, + // don't use try/catch to allow --abort-on-uncaught-exception + // to abort if an error is thrown. + // Otherwise, use try/catch to get the chance to emit an + // error on the current domain + if (!domain || domain.emittingTopLevelError) { + performNTCallback(args, callback); } else { - switch (args.length) { - case 1: - doNTCallback1(callback, args[0]); - break; - case 2: - doNTCallback2(callback, args[0], args[1]); - break; - case 3: - doNTCallback3(callback, args[0], args[1], args[2]); - break; - default: - doNTCallbackMany(callback, args); + try { + performNTCallback(args, callback); + } catch (err) { + process._fatalException(err); } } + if (1e4 < tickInfo[kIndex]) tickDone(); if (domain) @@ -398,7 +423,8 @@ tickDone(); _runMicrotasks(); emitPendingUnhandledRejections(); - } while (tickInfo[kLength] !== 0); + } while (!process._tickDoneForced && tickInfo[kLength] !== 0); + process._tickDoneForced = false; } function doNTCallback0(callback) { diff --git a/test/message/timeout_throw.out b/test/message/timeout_throw.out index 46d9cd2630142a..3fea7b34f9b655 100644 --- a/test/message/timeout_throw.out +++ b/test/message/timeout_throw.out @@ -3,4 +3,5 @@ ^ ReferenceError: undefined_reference_error_maker is not defined at null._onTimeout (*test*message*timeout_throw.js:*:*) + at fireTimer (timers.js:*:*) at Timer.listOnTimeout (timers.js:*:*) diff --git a/test/parallel/test-domain-top-level-error-handler-throw.js b/test/parallel/test-domain-top-level-error-handler-throw.js new file mode 100644 index 00000000000000..bc7e311e8f9422 --- /dev/null +++ b/test/parallel/test-domain-top-level-error-handler-throw.js @@ -0,0 +1,75 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +/* + * The goal of this test is to make sure that when a top-level error + * handler throws an error following the handling of a previous error, + * the process reports the error message from the error thrown in the + * top-level error handler, not the one from the previous error. + */ +'use strict'; + +var domainErrHandlerExMessage = 'exception from domain error handler'; +var internalExMessage = 'You should NOT see me'; + +if (process.argv[2] === 'child') { + var domain = require('domain'); + var d = domain.create(); + + d.on('error', function() { + throw new Error(domainErrHandlerExMessage); + }); + + d.run(function doStuff() { + process.nextTick(function() { + throw new Error(internalExMessage); + }); + }); +} else { + var fork = require('child_process').fork; + var assert = require('assert'); + + function test() { + var child = fork(process.argv[1], ['child'], {silent:true}); + var gotDataFromStderr = false; + var stderrOutput = ''; + if (child) { + child.stderr.on('data', function onStderrData(data) { + gotDataFromStderr = true; + stderrOutput += data.toString(); + }); + + child.on('exit', function onChildExited(exitCode, signal) { + assert(gotDataFromStderr); + assert(stderrOutput.indexOf(domainErrHandlerExMessage) !== -1); + assert(stderrOutput.indexOf(internalExMessage) === -1); + + var expectedExitCode = 7; + var expectedSignal = null; + + assert.equal(exitCode, expectedExitCode); + assert.equal(signal, expectedSignal); + }); + } + } + + test(); +} diff --git a/test/parallel/test-domain-with-abort-on-uncaught-exception.js b/test/parallel/test-domain-with-abort-on-uncaught-exception.js new file mode 100644 index 00000000000000..1f5ff4bd2629a4 --- /dev/null +++ b/test/parallel/test-domain-with-abort-on-uncaught-exception.js @@ -0,0 +1,230 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. +'use strict'; + +var assert = require('assert'); + +/* + * The goal of this test is to make sure that: + * + * - Even if --abort_on_uncaught_exception is passed on the command line, + * setting up a top-level domain error handler and throwing an error + * within this domain does *not* make the process abort. The process exits + * gracefully. + * + * - When passing --abort_on_uncaught_exception on the command line and + * setting up a top-level domain error handler, an error thrown + * within this domain's error handler *does* make the process abort. + * + * - When *not* passing --abort_on_uncaught_exception on the command line and + * setting up a top-level domain error handler, an error thrown within this + * domain's error handler does *not* make the process abort, but makes it exit + * with the proper failure exit code. + * + * - When throwing an error within the top-level domain's error handler + * within a try/catch block, the process should exit gracefully, whether or + * not --abort_on_uncaught_exception is passed on the command line. + */ + +var domainErrHandlerExMessage = 'exception from domain error handler'; + +if (process.argv[2] === 'child') { + var domain = require('domain'); + var d = domain.create(); + var triggeredProcessUncaughtException = false; + + process.on('uncaughtException', function onUncaughtException() { + // The process' uncaughtException event must not be emitted when + // an error handler is setup on the top-level domain. + // Exiting with exit code of 42 here so that it would assert when + // the parent checks the child exit code. + process.exit(42); + }); + + d.on('error', function(err) { + // Swallowing the error on purpose if 'throwInDomainErrHandler' is not + // set + if (process.argv.indexOf('throwInDomainErrHandler') !== -1) { + if (process.argv.indexOf('useTryCatch') !== -1) { + try { + throw new Error(domainErrHandlerExMessage); + } catch (e) { + } + } else { + throw new Error(domainErrHandlerExMessage); + } + } + }); + + d.run(function doStuff() { + // Throwing from within different types of callbacks as each of them + // handles domains differently + process.nextTick(function() { + throw new Error('Error from nextTick callback'); + }); + + var fs = require('fs'); + fs.exists('/non/existing/file', function onExists(exists) { + throw new Error('Error from fs.exists callback'); + }); + + setImmediate(function onSetImmediate() { + throw new Error('Error from setImmediate callback'); + }); + + throw new Error('Error from domain.run callback'); + }); +} else { + var exec = require('child_process').exec; + + function testDomainExceptionHandling(cmdLineOption, options) { + if (typeof cmdLineOption === 'object') { + options = cmdLineOption; + cmdLineOption = undefined; + } + + var throwInDomainErrHandlerOpt; + if (options.throwInDomainErrHandler) + throwInDomainErrHandlerOpt = 'throwInDomainErrHandler'; + + var cmdToExec = ''; + if (process.platform !== 'win32') { + // Do not create core files, as it can take a lot of disk space on + // continuous testing and developers' machines + cmdToExec += 'ulimit -c 0 && '; + } + + var useTryCatchOpt; + if (options.useTryCatch) + useTryCatchOpt = 'useTryCatch'; + + cmdToExec += process.argv[0] + ' '; + cmdToExec += (cmdLineOption ? cmdLineOption : '') + ' '; + cmdToExec += process.argv[1] + ' '; + cmdToExec += [ + 'child', + throwInDomainErrHandlerOpt, + useTryCatchOpt + ].join(' '); + + var child = exec(cmdToExec); + + if (child) { + var childTriggeredOnUncaughtExceptionHandler = false; + child.on('message', function onChildMsg(msg) { + if (msg === 'triggeredProcessUncaughtEx') { + childTriggeredOnUncaughtExceptionHandler = true; + } + }); + + child.on('exit', function onChildExited(exitCode, signal) { + var expectedExitCode = 0; + // We use an array of values since the actual signal can differ across + // compilers. + var expectedSignal = [null]; + + // 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) { + // 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 + 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. + // + // We use an array of values since the actual exit code can differ + // across compilers. + expectedExitCode = [132, 134]; + + // On Linux, v8 raises SIGTRAP when aborting because + // the "debug break" flag is on by default + if (process.platform === 'linux') + expectedExitCode.push(133); + + // 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 either + // SIGABRT or SIGILL. + if (process.platform === 'sunos') { + expectedExitCode = null; + expectedSignal = ['SIGABRT', 'SIGILL']; + } + + // On Windows, v8's base::OS::Abort also triggers a debug breakpoint + // which makes the process exit with code -2147483645 + if (process.platform === 'win32') + expectedExitCode = [-2147483645, 3221225477]; + } + } + + if (Array.isArray(expectedSignal)) { + assert.ok(expectedSignal.indexOf(signal) > -1); + } else { + assert.equal(signal, expectedSignal); + } + + if (Array.isArray(expectedExitCode)) { + assert.ok(expectedExitCode.indexOf(exitCode) > -1); + } else { + assert.equal(exitCode, expectedExitCode); + } + }); + } + } + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: false, + useTryCatch: false + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: false, + useTryCatch: true + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: true, + useTryCatch: false + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: true, + useTryCatch: true + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: false + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: false, + useTryCatch: false + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: true, + useTryCatch: true + }); +} diff --git a/test/parallel/test-repl-domain.js b/test/parallel/test-repl-domain.js index 7528f502878f63..eb8696fce4f770 100644 --- a/test/parallel/test-repl-domain.js +++ b/test/parallel/test-repl-domain.js @@ -5,6 +5,8 @@ var common = require('../common'); var util = require('util'); var repl = require('repl'); +var PROMPT = ''; + // A stream to push an array into a REPL function ArrayStream() { this.run = function(data) { @@ -21,16 +23,16 @@ ArrayStream.prototype.resume = function() {}; ArrayStream.prototype.write = function() {}; var putIn = new ArrayStream(); -var testMe = repl.start('', putIn); +var testMe = repl.start(PROMPT, putIn); putIn.write = function(data) { // Don't use assert for this because the domain might catch it, and // give a false negative. Don't throw, just print and exit. - if (data === 'OK\n') { + if (data === 'OK\n' || data === 'undefined\n' || data === PROMPT) { console.log('ok'); } else { - console.error(data); + console.error('error:', data); process.exit(1); } };