Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Commit f0453ca

Browse files
whitlockjcJulien Gilli
whitlockjc
authored and
Julien Gilli
committed
domains: port caeb677 from v0.10 to v0.12
caeb677 Do not abort the process if an error is thrown from within a domain, an error handler is setup for the domain and --abort-on-uncaught-exception was passed on the command line. However, if an error is thrown from within the top-level domain's error handler and --abort-on-uncaught-exception was passed on the command line, make the process abort. Fixes: #8877 Fixes: #8877 PR-URL: #25835 Reviewed-By: misterdjules - Julien Gilli <[email protected]>
1 parent 1982ed6 commit f0453ca

5 files changed

Lines changed: 399 additions & 29 deletions

File tree

‎lib/domain.js‎

Lines changed: 59 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,20 @@ Domain.prototype._disposed = undefined;
7777
// Called by process._fatalException in case an error was thrown.
7878
Domain.prototype._errorHandler = function errorHandler(er) {
7979
var caught = false;
80+
var self = this;
81+
82+
function emitError() {
83+
var handled = self.emit('error', er);
84+
85+
// Exit all domains on the stack. Uncaught exceptions end the
86+
// current tick and no domains should be left on the stack
87+
// between ticks.
88+
stack.length = 0;
89+
exports.active = process.domain = null;
90+
91+
return handled;
92+
}
93+
8094
// ignore errors on disposed domains.
8195
//
8296
// XXX This is a bit stupid. We should probably get rid of
@@ -89,38 +103,54 @@ Domain.prototype._errorHandler = function errorHandler(er) {
89103
er.domain = this;
90104
er.domainThrown = true;
91105
}
92-
// wrap this in a try/catch so we don't get infinite throwing
93-
try {
94-
// One of three things will happen here.
95-
//
96-
// 1. There is a handler, caught = true
97-
// 2. There is no handler, caught = false
98-
// 3. It throws, caught = false
99-
//
100-
// If caught is false after this, then there's no need to exit()
101-
// the domain, because we're going to crash the process anyway.
102-
caught = this.emit('error', er);
103106

104-
// Exit all domains on the stack. Uncaught exceptions end the
105-
// current tick and no domains should be left on the stack
106-
// between ticks.
107-
stack.length = 0;
108-
exports.active = process.domain = null;
109-
} catch (er2) {
110-
// The domain error handler threw! oh no!
111-
// See if another domain can catch THIS error,
112-
// or else crash on the original one.
113-
// If the user already exited it, then don't double-exit.
114-
if (this === exports.active) {
115-
stack.pop();
107+
// The top-level domain-handler is handled separately.
108+
//
109+
// The reason is that if V8 was passed a command line option
110+
// asking it to abort on an uncaught exception (currently
111+
// "--abort-on-uncaught-exception"), we want an uncaught exception
112+
// in the top-level domain error handler to make the
113+
// process abort. Using try/catch here would always make V8 think
114+
// that these exceptions are caught, and thus would prevent it from
115+
// aborting in these cases.
116+
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;
116125
}
117-
if (stack.length) {
118-
exports.active = process.domain = stack[stack.length - 1];
119-
caught = process._fatalException(er2);
120-
} else {
121-
caught = false;
126+
} else {
127+
// wrap this in a try/catch so we don't get infinite throwing
128+
try {
129+
// One of three things will happen here.
130+
//
131+
// 1. There is a handler, caught = true
132+
// 2. There is no handler, caught = false
133+
// 3. It throws, caught = false
134+
//
135+
// If caught is false after this, then there's no need to exit()
136+
// the domain, because we're going to crash the process anyway.
137+
caught = emitError();
138+
} catch (er2) {
139+
// The domain error handler threw! oh no!
140+
// See if another domain can catch THIS error,
141+
// or else crash on the original one.
142+
// If the user already exited it, then don't double-exit.
143+
if (this === exports.active) {
144+
stack.pop();
145+
}
146+
if (stack.length) {
147+
exports.active = process.domain = stack[stack.length - 1];
148+
caught = process._fatalException(er2);
149+
} else {
150+
caught = false;
151+
}
152+
return caught;
122153
}
123-
return caught;
124154
}
125155
return caught;
126156
};

‎src/env.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ namespace node {
8686
V(dev_string, "dev") \
8787
V(disposed_string, "_disposed") \
8888
V(domain_string, "domain") \
89+
V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError") \
8990
V(exchange_string, "exchange") \
9091
V(idle_string, "idle") \
9192
V(irq_string, "irq") \

‎src/node.cc‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -909,6 +909,33 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
909909
#endif
910910

911911

912+
static bool IsDomainActive(const Environment* env) {
913+
if (!env->using_domains()) {
914+
return false;
915+
}
916+
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)
920+
return false;
921+
922+
Local<Value> domain_v = domain_array->Get(0);
923+
return !domain_v->IsNull();
924+
}
925+
926+
927+
bool ShouldAbortOnUncaughtException(v8::Isolate* isolate) {
928+
Environment* env = Environment::GetCurrent(isolate);
929+
Local<Object> process_object = env->process_object();
930+
Local<String> emitting_top_level_domain_error_key =
931+
env->emitting_top_level_domain_error_string();
932+
bool isEmittingTopLevelDomainError =
933+
process_object->Get(emitting_top_level_domain_error_key)->BooleanValue();
934+
935+
return !IsDomainActive(env) || isEmittingTopLevelDomainError;
936+
}
937+
938+
912939
void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
913940
Environment* env = Environment::GetCurrent(args.GetIsolate());
914941

@@ -2772,6 +2799,9 @@ void SetupProcessObject(Environment* env,
27722799

27732800
// pre-set _events object for faster emit checks
27742801
process->Set(env->events_string(), Object::New(env->isolate()));
2802+
2803+
process->Set(env->emitting_top_level_domain_error_string(),
2804+
False(env->isolate()));
27752805
}
27762806

27772807

@@ -3453,6 +3483,8 @@ void Init(int* argc,
34533483
node_isolate = Isolate::New();
34543484
Isolate::Scope isolate_scope(node_isolate);
34553485

3486+
node_isolate->SetAbortOnUncaughtException(ShouldAbortOnUncaughtException);
3487+
34563488
#ifdef __POSIX__
34573489
// Raise the open file descriptor limit.
34583490
{ // NOLINT (whitespace/braces)

‎test/simple/test-domain-top-level-error-handler-throw.js‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Copyright Joyent, Inc. and other Node contributors.
2+
//
3+
// Permission is hereby granted, free of charge, to any person obtaining a
4+
// copy of this software and associated documentation files (the
5+
// "Software"), to deal in the Software without restriction, including
6+
// without limitation the rights to use, copy, modify, merge, publish,
7+
// distribute, sublicense, and/or sell copies of the Software, and to permit
8+
// persons to whom the Software is furnished to do so, subject to the
9+
// following conditions:
10+
//
11+
// The above copyright notice and this permission notice shall be included
12+
// in all copies or substantial portions of the Software.
13+
//
14+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
15+
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
16+
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
17+
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
18+
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
19+
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
20+
// USE OR OTHER DEALINGS IN THE SOFTWARE.
21+
22+
/*
23+
* The goal of this test is to make sure that when a top-level error
24+
* handler throws an error following the handling of a previous error,
25+
* the process reports the error message from the error thrown in the
26+
* top-level error handler, not the one from the previous error.
27+
*/
28+
29+
var domainErrHandlerExMessage = 'exception from domain error handler';
30+
var internalExMessage = 'You should NOT see me';
31+
32+
if (process.argv[2] === 'child') {
33+
var domain = require('domain');
34+
var d = domain.create();
35+
36+
d.on('error', function() {
37+
throw new Error(domainErrHandlerExMessage);
38+
});
39+
40+
d.run(function doStuff() {
41+
process.nextTick(function () {
42+
throw new Error(internalExMessage);
43+
});
44+
});
45+
} else {
46+
var fork = require('child_process').fork;
47+
var assert = require('assert');
48+
49+
function test() {
50+
var child = fork(process.argv[1], ['child'], {silent:true});
51+
var gotDataFromStderr = false;
52+
var stderrOutput = '';
53+
if (child) {
54+
child.stderr.on('data', function onStderrData(data) {
55+
gotDataFromStderr = true;
56+
stderrOutput += data.toString();
57+
});
58+
59+
child.on('exit', function onChildExited(exitCode, signal) {
60+
assert(gotDataFromStderr);
61+
assert(stderrOutput.indexOf(domainErrHandlerExMessage) !== -1);
62+
assert(stderrOutput.indexOf(internalExMessage) === -1);
63+
64+
var expectedExitCode = 7;
65+
var expectedSignal = null;
66+
67+
assert.equal(exitCode, expectedExitCode);
68+
assert.equal(signal, expectedSignal);
69+
});
70+
}
71+
}
72+
73+
test();
74+
}

0 commit comments

Comments
 (0)