Skip to content

Commit 007a7c2

Browse files
author
Julien Gilli
committed
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.
1 parent ca97fb6 commit 007a7c2

File tree

6 files changed

+703
-22
lines changed

6 files changed

+703
-22
lines changed

‎lib/domain.js

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -45,19 +45,20 @@ Object.defineProperty(process, 'domain', {
4545
// between js and c++ w/o much overhead
4646
var _domain_flag = {};
4747

48+
// it's possible to enter one domain while already inside
49+
// another one. the stack is each entered domain.
50+
var stack = [];
51+
exports._stack = stack;
52+
4853
// let the process know we're using domains
49-
process._setupDomainUse(_domain, _domain_flag);
54+
process._setupDomainUse(_domain, _domain_flag, stack);
5055

5156
exports.Domain = Domain;
5257

5358
exports.create = exports.createDomain = function() {
5459
return new Domain();
5560
};
5661

57-
// it's possible to enter one domain while already inside
58-
// another one. the stack is each entered domain.
59-
var stack = [];
60-
exports._stack = stack;
6162
// the active domain is always the one that we're currently in.
6263
exports.active = null;
6364

@@ -114,14 +115,20 @@ Domain.prototype._errorHandler = function errorHandler(er) {
114115
// that these exceptions are caught, and thus would prevent it from
115116
// aborting in these cases.
116117
if (stack.length === 1) {
117-
try {
118-
// Set the _emittingTopLevelDomainError so that we know that, even
119-
// if technically the top-level domain is still active, it would
120-
// be ok to abort on an uncaught exception at this point
121-
process._emittingTopLevelDomainError = true;
122-
caught = emitError();
123-
} finally {
124-
process._emittingTopLevelDomainError = false;
118+
// If there's no error handler, do not emit an 'error' event
119+
// as this would throw an error, make the process exit, and thus
120+
// prevent the process 'uncaughtException' event from being emitted
121+
// if a listener is set.
122+
if (EventEmitter.listenerCount(self, 'error') > 0) {
123+
try {
124+
// Set the _emittingTopLevelDomainError so that we know that, even
125+
// if technically the top-level domain is still active, it would
126+
// be ok to abort on an uncaught exception at this point
127+
process._emittingTopLevelDomainError = true;
128+
caught = emitError();
129+
} finally {
130+
process._emittingTopLevelDomainError = false;
131+
}
125132
}
126133
} else {
127134
// wrap this in a try/catch so we don't get infinite throwing

‎src/env.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ namespace node {
258258
V(buffer_constructor_function, v8::Function) \
259259
V(context, v8::Context) \
260260
V(domain_array, v8::Array) \
261+
V(domains_stack_array, v8::Array) \
261262
V(fs_stats_constructor_function, v8::Function) \
262263
V(gc_info_callback_function, v8::Function) \
263264
V(module_load_list_array, v8::Array) \

‎src/node.cc

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -908,19 +908,53 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
908908
}
909909
#endif
910910

911+
static bool DomainHasErrorHandler(const Environment* env,
912+
const Local<Object>& domain) {
913+
HandleScope scope(env->isolate());
911914

912-
static bool IsDomainActive(const Environment* env) {
913-
if (!env->using_domains()) {
915+
Local<Value> domain_event_listeners_v = domain->Get(env->events_string());
916+
if (!domain_event_listeners_v->IsObject())
914917
return false;
915-
}
916918

917-
Local<Array> domain_array = env->domain_array().As<Array>();
918-
uint32_t domains_array_length = domain_array->Length();
919-
if (domains_array_length == 0)
919+
Local<Object> domain_event_listeners_o =
920+
domain_event_listeners_v.As<Object>();
921+
922+
Local<Value> domain_error_listeners_v =
923+
domain_event_listeners_o->Get(env->error_string());
924+
925+
if (domain_error_listeners_v->IsFunction() ||
926+
(domain_error_listeners_v->IsArray() &&
927+
domain_error_listeners_v.As<Array>()->Length() > 0))
928+
return true;
929+
930+
return false;
931+
}
932+
933+
static bool TopDomainHasErrorHandler(const Environment* env) {
934+
HandleScope scope(env->isolate());
935+
936+
if (!env->using_domains())
920937
return false;
921938

922-
Local<Value> domain_v = domain_array->Get(0);
923-
return !domain_v->IsNull();
939+
Local<Array> domains_stack_array = env->domains_stack_array().As<Array>();
940+
if (domains_stack_array->Length() == 0)
941+
return false;
942+
943+
uint32_t domains_stack_length = domains_stack_array->Length();
944+
if (domains_stack_length == 0)
945+
return false;
946+
947+
Local<Value> top_domain_v =
948+
domains_stack_array->Get(domains_stack_length - 1);
949+
950+
if (!top_domain_v->IsObject())
951+
return false;
952+
953+
Local<Object> top_domain = top_domain_v.As<Object>();
954+
if (DomainHasErrorHandler(env, top_domain))
955+
return true;
956+
957+
return false;
924958
}
925959

926960

@@ -932,7 +966,7 @@ bool ShouldAbortOnUncaughtException(v8::Isolate* isolate) {
932966
bool isEmittingTopLevelDomainError =
933967
process_object->Get(emitting_top_level_domain_error_key)->BooleanValue();
934968

935-
return !IsDomainActive(env) || isEmittingTopLevelDomainError;
969+
return isEmittingTopLevelDomainError || !TopDomainHasErrorHandler(env);
936970
}
937971

938972

@@ -960,8 +994,10 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
960994

961995
assert(args[0]->IsArray());
962996
assert(args[1]->IsObject());
997+
assert(args[2]->IsArray());
963998

964999
env->set_domain_array(args[0].As<Array>());
1000+
env->set_domains_stack_array(args[2].As<Array>());
9651001

9661002
Local<Object> domain_flag_obj = args[1].As<Object>();
9671003
Environment::DomainFlag* domain_flag = env->domain_flag();

‎test/simple/test-domain-abort-on-uncaught.js

Lines changed: 243 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,243 @@
1+
'use strict';
2+
3+
// This test makes sure that when using --abort-on-uncaught-exception and
4+
// when throwing an error from within a domain that has an error handler
5+
// setup, the process _does not_ abort.
6+
7+
var common = require('../common');
8+
var assert = require('assert');
9+
var domain = require('domain');
10+
var child_process = require('child_process');
11+
12+
var errorHandlerCalled = false;
13+
14+
var tests = [
15+
function nextTick() {
16+
var d = domain.create();
17+
18+
d.once('error', function(err) {
19+
errorHandlerCalled = true;
20+
});
21+
22+
d.run(function() {
23+
process.nextTick(function() {
24+
throw new Error('exceptional!');
25+
});
26+
});
27+
},
28+
29+
function timer() {
30+
var d = domain.create();
31+
32+
d.on('error', function(err) {
33+
errorHandlerCalled = true;
34+
});
35+
36+
d.run(function() {
37+
setTimeout(function() {
38+
throw new Error('exceptional!');
39+
}, 33);
40+
});
41+
},
42+
43+
function immediate() {
44+
console.log('starting test');
45+
var d = domain.create();
46+
47+
d.on('error', function errorHandler() {
48+
errorHandlerCalled = true;
49+
});
50+
51+
d.run(function() {
52+
setImmediate(function() {
53+
throw new Error('boom!');
54+
});
55+
});
56+
},
57+
58+
function timerPlusNextTick() {
59+
var d = domain.create();
60+
61+
d.on('error', function(err) {
62+
errorHandlerCalled = true;
63+
});
64+
65+
d.run(function() {
66+
setTimeout(function() {
67+
process.nextTick(function() {
68+
throw new Error('exceptional!');
69+
});
70+
}, 33);
71+
});
72+
},
73+
74+
function firstRun() {
75+
var d = domain.create();
76+
77+
d.on('error', function(err) {
78+
errorHandlerCalled = true;
79+
});
80+
81+
d.run(function() {
82+
throw new Error('exceptional!');
83+
});
84+
},
85+
86+
function fsAsync() {
87+
var d = domain.create();
88+
89+
d.on('error', function errorHandler() {
90+
errorHandlerCalled = true;
91+
});
92+
93+
d.run(function() {
94+
var fs = require('fs');
95+
fs.exists('/non/existing/file', function onExists(exists) {
96+
throw new Error('boom!');
97+
});
98+
});
99+
},
100+
101+
function netServer() {
102+
var net = require('net');
103+
var d = domain.create();
104+
105+
d.on('error', function(err) {
106+
errorHandlerCalled = true;
107+
});
108+
109+
d.run(function() {
110+
var server = net.createServer(function(conn) {
111+
conn.pipe(conn);
112+
});
113+
server.listen(common.PORT, '0.0.0.0', function() {
114+
var conn = net.connect(common.PORT, '0.0.0.0');
115+
conn.once('data', function() {
116+
throw new Error('ok');
117+
});
118+
conn.end('ok');
119+
server.close();
120+
});
121+
});
122+
},
123+
124+
function firstRunNestedWithErrorHandler() {
125+
var d = domain.create();
126+
var d2 = domain.create();
127+
128+
d2.on('error', function errorHandler() {
129+
errorHandlerCalled = true;
130+
});
131+
132+
d.run(function() {
133+
d2.run(function() {
134+
throw new Error('boom!');
135+
});
136+
});
137+
},
138+
139+
function timeoutNestedWithErrorHandler() {
140+
var d = domain.create();
141+
var d2 = domain.create();
142+
143+
d2.on('error', function errorHandler() {
144+
errorHandlerCalled = true;
145+
});
146+
147+
d.run(function() {
148+
d2.run(function() {
149+
setTimeout(function() {
150+
console.log('foo');
151+
throw new Error('boom!');
152+
}, 33);
153+
});
154+
});
155+
},
156+
157+
function setImmediateNestedWithErrorHandler() {
158+
var d = domain.create();
159+
var d2 = domain.create();
160+
161+
d2.on('error', function errorHandler() {
162+
errorHandlerCalled = true;
163+
});
164+
165+
d.run(function() {
166+
d2.run(function() {
167+
setImmediate(function() {
168+
throw new Error('boom!');
169+
});
170+
});
171+
});
172+
},
173+
174+
function nextTickNestedWithErrorHandler() {
175+
var d = domain.create();
176+
var d2 = domain.create();
177+
178+
d2.on('error', function errorHandler() {
179+
errorHandlerCalled = true;
180+
});
181+
182+
d.run(function() {
183+
d2.run(function() {
184+
process.nextTick(function() {
185+
throw new Error('boom!');
186+
});
187+
});
188+
});
189+
},
190+
191+
function fsAsyncNestedWithErrorHandler() {
192+
var d = domain.create();
193+
var d2 = domain.create();
194+
195+
d2.on('error', function errorHandler() {
196+
errorHandlerCalled = true;
197+
});
198+
199+
d.run(function() {
200+
d2.run(function() {
201+
var fs = require('fs');
202+
fs.exists('/non/existing/file', function onExists(exists) {
203+
throw new Error('boom!');
204+
});
205+
});
206+
});
207+
}
208+
];
209+
210+
if (process.argv[2] === 'child') {
211+
var testIndex = +process.argv[3];
212+
213+
tests[testIndex]();
214+
215+
process.on('exit', function onExit() {
216+
assert.equal(errorHandlerCalled, true);
217+
});
218+
} else {
219+
220+
tests.forEach(function(test, testIndex) {
221+
var testCmd = '';
222+
if (process.platform !== 'win32') {
223+
// Do not create core files, as it can take a lot of disk space on
224+
// continuous testing and developers' machines
225+
testCmd += 'ulimit -c 0 && ';
226+
}
227+
228+
testCmd += process.argv[0];
229+
testCmd += ' ' + '--abort-on-uncaught-exception';
230+
testCmd += ' ' + process.argv[1];
231+
testCmd += ' ' + 'child';
232+
testCmd += ' ' + testIndex;
233+
234+
var child = child_process.exec(testCmd);
235+
236+
child.on('exit', function onExit(code, signal) {
237+
assert.equal(code, 0, 'Test at index ' + testIndex +
238+
' should have exited with exit code 0 but instead exited with code ' +
239+
code + ' and signal ' + signal);
240+
});
241+
242+
});
243+
}

0 commit comments

Comments
 (0)