From 82ce3a6cba6766651cd4aff19105a4c58f864ba6 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Mon, 2 Nov 2015 16:01:51 -0800 Subject: [PATCH] domains: fix handling of uncaught exceptions Fix node exiting due to an exception being thrown rather than emitting an 'uncaughtException' event on the process object when: 1. no error handler is set on the domain within which an error is thrown 2. an 'uncaughtException' event listener is set on the process Also fix an issue where the process would not abort in the proper function call if an error is thrown within a domain with no error handler and --abort-on-uncaught-exception is used. Fixes #3607 and #3653. --- lib/domain.js | 11 +- src/node.cc | 76 +++++- src/node.js | 22 +- test/simple/test-domain-abort-on-uncaught.js | 241 ++++++++++++++++++ ...main-no-error-handler-abort-on-uncaught.js | 189 ++++++++++++++ test/simple/test-domain-uncaught-exception.js | 205 +++++++++++++++ 6 files changed, 726 insertions(+), 18 deletions(-) create mode 100644 test/simple/test-domain-abort-on-uncaught.js create mode 100644 test/simple/test-domain-no-error-handler-abort-on-uncaught.js create mode 100644 test/simple/test-domain-uncaught-exception.js diff --git a/lib/domain.js b/lib/domain.js index afaa922ebe5919..7edb6012ccd86d 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -32,8 +32,13 @@ var endMethods = ['end', 'abort', 'destroy', 'destroySoon']; // a few side effects. events.usingDomains = true; +// it's possible to enter one domain while already inside +// another one. the stack is each entered domain. +var stack = []; +exports._stack = stack; + // let the process know we're using domains -process._usingDomains(); +process._usingDomains(stack); exports.Domain = Domain; @@ -41,10 +46,6 @@ exports.create = exports.createDomain = function(cb) { return new Domain(cb); }; -// it's possible to enter one domain while already inside -// another one. the stack is each entered domain. -var stack = []; -exports._stack = stack; // the active domain is always the one that we're currently in. exports.active = null; diff --git a/src/node.cc b/src/node.cc index a8e1384e648f5f..3ce317304833cb 100644 --- a/src/node.cc +++ b/src/node.cc @@ -105,6 +105,8 @@ Persistent domain_symbol; // declared in node_internals.h Persistent process; +static Persistent domains_stack; + static Persistent process_tickFromSpinner; static Persistent process_tickCallback; @@ -126,6 +128,8 @@ static Persistent exit_symbol; static Persistent disposed_symbol; static Persistent emitting_toplevel_domain_error_symbol; +static Persistent _events_symbol; +static Persistent error_symbol; static bool print_eval = false; static bool force_repl = false; @@ -904,25 +908,77 @@ Handle FromConstructorTemplate(Persistent t, return scope.Close(t->GetFunction()->NewInstance(argc, argv)); } -static bool IsDomainActive() { - if (domain_symbol.IsEmpty()) - domain_symbol = NODE_PSYMBOL("domain"); +static bool DomainHasErrorHandler(const Local& domain) { + HandleScope scope; + + if (_events_symbol.IsEmpty()) + _events_symbol = NODE_PSYMBOL("_events"); + + Local domain_event_listeners_v = domain->Get(_events_symbol); + if (!domain_event_listeners_v->IsObject()) + return false; + + Local domain_event_listeners_o = + domain_event_listeners_v.As(); + + if (error_symbol.IsEmpty()) + error_symbol = NODE_PSYMBOL("error"); + + Local domain_error_listeners_v = + domain_event_listeners_o->Get(error_symbol); + + if (domain_error_listeners_v->IsFunction() || + (domain_error_listeners_v->IsArray() && + domain_error_listeners_v.As()->Length() > 0)) + return true; + + return false; +} + +static bool TopDomainHasErrorHandler() { + HandleScope scope; + + if (!using_domains) + return false; - Local domain_v = process->Get(domain_symbol); + Local domains_stack_array = Local::New(domains_stack); + if (domains_stack_array->Length() == 0) + return false; - return domain_v->IsObject(); + uint32_t domains_stack_length = domains_stack_array->Length(); + if (domains_stack_length == 0) + return false; + + Local domain_v = domains_stack_array->Get(domains_stack_length - 1); + if (!domain_v->IsObject()) + return false; + + Local domain = domain_v.As(); + if (DomainHasErrorHandler(domain)) + return true; + + return false; } bool ShouldAbortOnUncaughtException() { Local emitting_toplevel_domain_error_v = process->Get(emitting_toplevel_domain_error_symbol); - return !IsDomainActive() || emitting_toplevel_domain_error_v->BooleanValue(); + + return emitting_toplevel_domain_error_v->BooleanValue() || + !TopDomainHasErrorHandler(); } Handle UsingDomains(const Arguments& args) { HandleScope scope; + if (using_domains) return scope.Close(Undefined()); + + if (!args[0]->IsArray()) { + fprintf(stderr, "domains stack must be an array\n"); + abort(); + } + using_domains = true; Local tdc_v = process->Get(String::New("_tickDomainCallback")); Local ndt_v = process->Get(String::New("_nextDomainTick")); @@ -934,6 +990,9 @@ Handle UsingDomains(const Arguments& args) { fprintf(stderr, "process._nextDomainTick assigned to non-function\n"); abort(); } + + domains_stack = Persistent::New(args[0].As()); + Local tdc = tdc_v.As(); Local ndt = ndt_v.As(); process->Set(String::New("_tickCallback"), tdc); @@ -2449,7 +2508,10 @@ Handle SetupProcessObject(int argc, char *argv[]) { process->Set(String::NewSymbol("_tickInfoBox"), info_box); // pre-set _events object for faster emit checks - process->Set(String::NewSymbol("_events"), Object::New()); + if (_events_symbol.IsEmpty()) + _events_symbol = NODE_PSYMBOL("_events"); + + process->Set(_events_symbol, Object::New()); if (emitting_toplevel_domain_error_symbol.IsEmpty()) emitting_toplevel_domain_error_symbol = diff --git a/src/node.js b/src/node.js index ab560405fa4590..cbee98595540c9 100644 --- a/src/node.js +++ b/src/node.js @@ -229,6 +229,7 @@ // and exit if there are no listeners. process._fatalException = function(er) { var caught = false; + if (process.domain) { var domain = process.domain; var domainModule = NativeModule.require('domain'); @@ -256,11 +257,17 @@ // aborting in these cases. if (domainStack.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 = domain.emit('error', er); + // 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 (process.EventEmitter.listenerCount(domain, 'error') > 0) { + // 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 = domain.emit('error', er); + } } finally { process._emittingTopLevelDomainError = false; } @@ -297,9 +304,12 @@ // current tick and no domains should be left on the stack // between ticks. _clearDomainsStack(); - } else { + } + + if (!caught) { caught = process.emit('uncaughtException', er); } + // if someone handled it, then great. otherwise, die in C++ land // since that means that we'll exit the process, emit the 'exit' event if (!caught) { diff --git a/test/simple/test-domain-abort-on-uncaught.js b/test/simple/test-domain-abort-on-uncaught.js new file mode 100644 index 00000000000000..ad75be74ea8d67 --- /dev/null +++ b/test/simple/test-domain-abort-on-uncaught.js @@ -0,0 +1,241 @@ +'use strict'; + +// This test makes sure that when using --abort-on-uncaught-exception and +// when throwing an error from within a domain that has an error handler +// setup, the process _does not_ abort. + +var common = require('../common'); +var assert = require('assert'); +var domain = require('domain'); +var child_process = require('child_process'); + +var errorHandlerCalled = false; + +var tests = [ + function nextTick() { + var d = domain.create(); + + d.once('error', function(err) { + errorHandlerCalled = true; + }); + + d.run(function() { + process.nextTick(function() { + throw new Error('exceptional!'); + }); + }); + }, + + function timer() { + var d = domain.create(); + + d.on('error', function(err) { + errorHandlerCalled = true; + }); + + d.run(function() { + setTimeout(function() { + throw new Error('exceptional!'); + }, 33); + }); + }, + + function immediate() { + var d = domain.create(); + + d.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + setImmediate(function() { + throw new Error('boom!'); + }); + }); + }, + + function timerPlusNextTick() { + var d = domain.create(); + + d.on('error', function(err) { + errorHandlerCalled = true; + }); + + d.run(function() { + setTimeout(function() { + process.nextTick(function() { + throw new Error('exceptional!'); + }); + }, 33); + }); + }, + + function firstRun() { + var d = domain.create(); + + d.on('error', function(err) { + errorHandlerCalled = true; + }); + + d.run(function() { + throw new Error('exceptional!'); + }); + }, + + function fsAsync() { + var d = domain.create(); + + d.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + var fs = require('fs'); + fs.exists('/non/existing/file', function onExists(exists) { + throw new Error('boom!'); + }); + }); + }, + + function netServer() { + var net = require('net'); + var d = domain.create(); + + d.on('error', function(err) { + errorHandlerCalled = true; + }); + + d.run(function() { + var server = net.createServer(function(conn) { + conn.pipe(conn); + }); + server.listen(common.PORT, '0.0.0.0', function() { + var conn = net.connect(common.PORT, '0.0.0.0'); + conn.once('data', function() { + throw new Error('ok'); + }); + conn.end('ok'); + server.close(); + }); + }); + }, + + function firstRunNestedWithErrorHandler() { + var d = domain.create(); + var d2 = domain.create(); + + d2.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + d2.run(function() { + throw new Error('boom!'); + }); + }); + }, + + function timeoutNestedWithErrorHandler() { + var d = domain.create(); + var d2 = domain.create(); + + d2.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + d2.run(function() { + setTimeout(function() { + console.log('foo'); + throw new Error('boom!'); + }, 33); + }); + }); + }, + + function setImmediateNestedWithErrorHandler() { + var d = domain.create(); + var d2 = domain.create(); + + d2.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + d2.run(function() { + setImmediate(function() { + throw new Error('boom!'); + }); + }); + }); + }, + + function nextTickNestedWithErrorHandler() { + var d = domain.create(); + var d2 = domain.create(); + + d2.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + d2.run(function() { + process.nextTick(function() { + throw new Error('boom!'); + }); + }); + }); + }, + + function fsAsyncNestedWithErrorHandler() { + var d = domain.create(); + var d2 = domain.create(); + + d2.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + d2.run(function() { + var fs = require('fs'); + fs.exists('/non/existing/file', function onExists(exists) { + throw new Error('boom!'); + }); + }); + }); + } +]; + +if (process.argv[2] === 'child') { + var testIndex = +process.argv[3]; + + tests[testIndex](); + + process.on('exit', function onExit() { + assert.equal(errorHandlerCalled, true); + }); +} else { + + tests.forEach(function(test, testIndex) { + 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]; + testCmd += ' ' + '--abort-on-uncaught-exception'; + testCmd += ' ' + process.argv[1]; + testCmd += ' ' + 'child'; + testCmd += ' ' + testIndex; + + var child = child_process.exec(testCmd); + + child.on('exit', function onExit(code, signal) { + assert.equal(code, 0, 'Test at index ' + testIndex + + ' should have exited with exit code 0 but instead exited with code ' + + code + ' and signal ' + signal); + }); + }); +} 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 new file mode 100644 index 00000000000000..78b8b764432019 --- /dev/null +++ b/test/simple/test-domain-no-error-handler-abort-on-uncaught.js @@ -0,0 +1,189 @@ +'use strict'; + +/* + * This test makes sure that when using --abort-on-uncaught-exception and + * when throwing an error from within a domain that does not have an error + * handler setup, the process aborts. + */ +var common = require('../common'); +var assert = require('assert'); +var domain = require('domain'); +var child_process = require('child_process'); + +var tests = [ + function() { + var d = domain.create(); + + d.run(function() { + throw new Error('boom!'); + }); + }, + + function() { + var d = domain.create(); + var d2 = domain.create(); + + d.run(function() { + d2.run(function() { + throw new Error('boom!'); + }); + }); + }, + + /* + * In this case, only the domain at the top of the stack "d" has an error + * handler, but the domain from which the error is thrown doesn't have an + * error handler. To be consistent with the way domains bubble up errors, + * this test case _should not_ make the process abort. However it was + * determined that making such a change in v0.10.x might be a breaking + * change, so instead this test case makes sure the process aborts. + */ + function() { + var d = domain.create(); + var d2 = domain.create(); + + d.on('error', function errorHandler() { + }); + + d.run(function() { + d2.run(function() { + throw new Error('boom!'); + }); + }); + }, + + function() { + var d = domain.create(); + + d.run(function() { + setTimeout(function() { + throw new Error('boom!'); + }); + }); + }, + + function() { + var d = domain.create(); + + d.run(function() { + setImmediate(function() { + throw new Error('boom!'); + }); + }); + }, + + function() { + var d = domain.create(); + + d.run(function() { + process.nextTick(function() { + throw new Error('boom!'); + }); + }); + }, + + function() { + var d = domain.create(); + + d.run(function() { + var fs = require('fs'); + fs.exists('/non/existing/file', function onExists(exists) { + throw new Error('boom!'); + }); + }); + }, + + function() { + var d = domain.create(); + var d2 = domain.create(); + + d.on('error', function errorHandler() { + }); + + d.run(function() { + d2.run(function() { + setTimeout(function() { + throw new Error('boom!'); + }); + }); + }); + }, + + function() { + var d = domain.create(); + var d2 = domain.create(); + + d.on('error', function errorHandler() { + }); + + d.run(function() { + d2.run(function() { + setImmediate(function() { + throw new Error('boom!'); + }); + }); + }); + }, + + function() { + var d = domain.create(); + var d2 = domain.create(); + + d.on('error', function errorHandler() { + }); + + d.run(function() { + d2.run(function() { + process.nextTick(function() { + throw new Error('boom!'); + }); + }); + }); + }, + + function() { + var d = domain.create(); + var d2 = domain.create(); + + d.on('error', function errorHandler() { + }); + + d.run(function() { + d2.run(function() { + var fs = require('fs'); + fs.exists('/non/existing/file', function onExists(exists) { + throw new Error('boom!'); + }); + }); + }); + }, +]; + +if (process.argv[2] === 'child') { + var testIndex = +process.argv[3]; + tests[testIndex](); +} else { + + tests.forEach(function(test, testIndex) { + 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]; + testCmd += ' ' + '--abort-on-uncaught-exception'; + testCmd += ' ' + process.argv[1]; + testCmd += ' ' + 'child'; + testCmd += ' ' + testIndex; + + 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); + }); + }); +} diff --git a/test/simple/test-domain-uncaught-exception.js b/test/simple/test-domain-uncaught-exception.js new file mode 100644 index 00000000000000..a9ec85dfa67708 --- /dev/null +++ b/test/simple/test-domain-uncaught-exception.js @@ -0,0 +1,205 @@ +'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. + */ + +var common = require('../common'); +var assert = require('assert'); +var domain = require('domain'); +var child_process = require('child_process'); + +var uncaughtExceptions = {}; + +var 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. + */ + var 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. + */ + var 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. + */ + var d3 = domain.create(); + var 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. + */ + var d5 = domain.create(); + var 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. + */ + var d7 = domain.create(); + var 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. + */ + var d9 = domain.create(); + var 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') { + var 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) { + var 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 not have sent message: ' + receivedMessage + + ' but did'); + } + }); + } + }) + }); +}