From e7e1dfd208df293b55f92276f5940d99b9048f11 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Wed, 4 Nov 2015 11:16:22 -0800 Subject: [PATCH] domains: fix no error handler & abort-on-uncaught Make the process abort if an error is thrown within a domain with no error handler and `--abort-on-uncaught-exception` is used. If the domain within which the error is thrown has no error handler, but a domain further down the domains stack has one, the process will not abort. Fixes #3653. --- lib/domain.js | 11 +- src/node.cc | 81 +++++- test/simple/test-domain-abort-on-uncaught.js | 257 ++++++++++++++++++ ...main-no-error-handler-abort-on-uncaught.js | 167 ++++++++++++ test/simple/test-domain-uncaught-exception.js | 7 +- 5 files changed, 508 insertions(+), 15 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 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..40d18337a70dd0 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,82 @@ 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->ToObject(); + + if (domain_event_listeners_o->IsNull()) + return false; + + 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 domainsStackHasErrorHandler() { + 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(); + for (int i = domains_stack_length - 1; i >= 0; --i) { + Local domain_v = domains_stack_array->Get(i); + if (domain_v->IsNull()) + return false; + + Local domain = domain_v->ToObject(); + if (domain->IsNull()) + return false; + + 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() || + !domainsStackHasErrorHandler(); } 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 +995,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 +2513,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/test/simple/test-domain-abort-on-uncaught.js b/test/simple/test-domain-abort-on-uncaught.js new file mode 100644 index 00000000000000..f1382855b83014 --- /dev/null +++ b/test/simple/test-domain-abort-on-uncaught.js @@ -0,0 +1,257 @@ +'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 firstRunOnlyTopLevelErrorHandler() { + var d = domain.create(); + var d2 = domain.create(); + + d.on('error', function errorHandler() { + errorHandlerCalled = true; + }); + + d.run(function() { + d2.run(function() { + throw new Error('boom!'); + }); + }); + }, + + 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..d3277ea3e55409 --- /dev/null +++ b/test/simple/test-domain-no-error-handler-abort-on-uncaught.js @@ -0,0 +1,167 @@ +'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!'); + }); + }); + }, + + 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 index 4248766e666cd7..9b5562872190b8 100644 --- a/test/simple/test-domain-uncaught-exception.js +++ b/test/simple/test-domain-uncaught-exception.js @@ -186,14 +186,15 @@ if (process.argv[2] === 'child') { test.expectedMessages.forEach(function(expectedMessage) { if (test.messagesReceived === undefined || test.messagesReceived.indexOf(expectedMessage) === -1) - assert(false, 'test should have sent message: ' + expectedMessage + - ' but didn\'t'); + 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 should have sent message: ' + receivedMessage + + assert(false, 'test ' + test.fn.name + + ' should not have sent message: ' + receivedMessage + ' but did'); } });