Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 2cbbaaf

Browse filesBrowse files
trevnorrisevanlucas
trevnorris
authored and
evanlucas
committed
async_wrap: don't abort on callback exception
Rather than abort if the init/pre/post/final/destroy callbacks throw, force the exception to propagate and not be made catchable. This way the application is still not allowed to proceed but also allowed the location of the failure to print before exiting. Though the stack itself may not be of much use since all callbacks except init are called from the bottom of the call stack. /tmp/async-test.js:14 throw new Error('pre'); ^ Error: pre at InternalFieldObject.pre (/tmp/async-test.js:14:9) PR-URL: #5756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
1 parent 6f16882 commit 2cbbaaf
Copy full SHA for 2cbbaaf

File tree

Expand file treeCollapse file tree

5 files changed

+134
-12
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+134
-12
lines changed
Open diff view settings
Collapse file

‎src/async-wrap-inl.h‎

Copy file name to clipboardExpand all lines: src/async-wrap-inl.h
+11-4Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,15 @@ inline AsyncWrap::AsyncWrap(Environment* env,
5151
argv[3] = parent->object();
5252
}
5353

54+
v8::TryCatch try_catch(env->isolate());
55+
5456
v8::MaybeLocal<v8::Value> ret =
5557
init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv);
5658

57-
if (ret.IsEmpty())
58-
FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");
59+
if (ret.IsEmpty()) {
60+
ClearFatalExceptionHandlers(env);
61+
FatalException(env->isolate(), try_catch);
62+
}
5963

6064
bits_ |= 1; // ran_init_callback() is true now.
6165
}
@@ -69,10 +73,13 @@ inline AsyncWrap::~AsyncWrap() {
6973
if (!fn.IsEmpty()) {
7074
v8::HandleScope scope(env()->isolate());
7175
v8::Local<v8::Value> uid = v8::Integer::New(env()->isolate(), get_uid());
76+
v8::TryCatch try_catch(env()->isolate());
7277
v8::MaybeLocal<v8::Value> ret =
7378
fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid);
74-
if (ret.IsEmpty())
75-
FatalError("node::AsyncWrap::~AsyncWrap", "destroy hook threw");
79+
if (ret.IsEmpty()) {
80+
ClearFatalExceptionHandlers(env());
81+
FatalException(env()->isolate(), try_catch);
82+
}
7683
}
7784
}
7885

Collapse file

‎src/async-wrap.cc‎

Copy file name to clipboardExpand all lines: src/async-wrap.cc
+16-4Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ using v8::HeapProfiler;
1818
using v8::Integer;
1919
using v8::Isolate;
2020
using v8::Local;
21+
using v8::MaybeLocal;
2122
using v8::Object;
2223
using v8::RetainedObjectInfo;
2324
using v8::TryCatch;
@@ -225,17 +226,28 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
225226
}
226227

227228
if (ran_init_callback() && !pre_fn.IsEmpty()) {
228-
if (pre_fn->Call(context, 1, &uid).IsEmpty())
229-
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
229+
TryCatch try_catch(env()->isolate());
230+
MaybeLocal<Value> ar = pre_fn->Call(env()->context(), context, 1, &uid);
231+
if (ar.IsEmpty()) {
232+
ClearFatalExceptionHandlers(env());
233+
FatalException(env()->isolate(), try_catch);
234+
return Local<Value>();
235+
}
230236
}
231237

232238
Local<Value> ret = cb->Call(context, argc, argv);
233239

234240
if (ran_init_callback() && !post_fn.IsEmpty()) {
235241
Local<Value> did_throw = Boolean::New(env()->isolate(), ret.IsEmpty());
236242
Local<Value> vals[] = { uid, did_throw };
237-
if (post_fn->Call(context, ARRAY_SIZE(vals), vals).IsEmpty())
238-
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
243+
TryCatch try_catch(env()->isolate());
244+
MaybeLocal<Value> ar =
245+
post_fn->Call(env()->context(), context, ARRAY_SIZE(vals), vals);
246+
if (ar.IsEmpty()) {
247+
ClearFatalExceptionHandlers(env());
248+
FatalException(env()->isolate(), try_catch);
249+
return Local<Value>();
250+
}
239251
}
240252

241253
if (ret.IsEmpty()) {
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+34-4Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,8 +1175,13 @@ Local<Value> MakeCallback(Environment* env,
11751175
}
11761176

11771177
if (ran_init_callback && !pre_fn.IsEmpty()) {
1178-
if (pre_fn->Call(object, 0, nullptr).IsEmpty())
1179-
FatalError("node::MakeCallback", "pre hook threw");
1178+
TryCatch try_catch(env->isolate());
1179+
MaybeLocal<Value> ar = pre_fn->Call(env->context(), object, 0, nullptr);
1180+
if (ar.IsEmpty()) {
1181+
ClearFatalExceptionHandlers(env);
1182+
FatalException(env->isolate(), try_catch);
1183+
return Local<Value>();
1184+
}
11801185
}
11811186

11821187
Local<Value> ret = callback->Call(recv, argc, argv);
@@ -1187,8 +1192,14 @@ Local<Value> MakeCallback(Environment* env,
11871192
// This needs to be fixed.
11881193
Local<Value> vals[] =
11891194
{ Undefined(env->isolate()).As<Value>(), did_throw };
1190-
if (post_fn->Call(object, ARRAY_SIZE(vals), vals).IsEmpty())
1191-
FatalError("node::MakeCallback", "post hook threw");
1195+
TryCatch try_catch(env->isolate());
1196+
MaybeLocal<Value> ar =
1197+
post_fn->Call(env->context(), object, ARRAY_SIZE(vals), vals);
1198+
if (ar.IsEmpty()) {
1199+
ClearFatalExceptionHandlers(env);
1200+
FatalException(env->isolate(), try_catch);
1201+
return Local<Value>();
1202+
}
11921203
}
11931204

11941205
if (ret.IsEmpty()) {
@@ -2363,6 +2374,25 @@ void OnMessage(Local<Message> message, Local<Value> error) {
23632374
}
23642375

23652376

2377+
void ClearFatalExceptionHandlers(Environment* env) {
2378+
Local<Object> process = env->process_object();
2379+
Local<Value> events =
2380+
process->Get(env->context(), env->events_string()).ToLocalChecked();
2381+
2382+
if (events->IsObject()) {
2383+
events.As<Object>()->Set(
2384+
env->context(),
2385+
OneByteString(env->isolate(), "uncaughtException"),
2386+
Undefined(env->isolate())).FromJust();
2387+
}
2388+
2389+
process->Set(
2390+
env->context(),
2391+
env->domain_string(),
2392+
Undefined(env->isolate())).FromJust();
2393+
}
2394+
2395+
23662396
static void Binding(const FunctionCallbackInfo<Value>& args) {
23672397
Environment* env = Environment::GetCurrent(args);
23682398

Collapse file

‎src/node_internals.h‎

Copy file name to clipboardExpand all lines: src/node_internals.h
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,11 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
234234
Environment* env_;
235235
};
236236

237+
// Clear any domain and/or uncaughtException handlers to force the error's
238+
// propagation and shutdown the process. Use this to force the process to exit
239+
// by clearing all callbacks that could handle the error.
240+
void ClearFatalExceptionHandlers(Environment* env);
241+
237242
enum NodeInstanceType { MAIN, WORKER };
238243

239244
class NodeInstanceData {
Collapse file

‎test/parallel/test-async-wrap-throw-from-callback.js‎

Copy file name to clipboard
+68Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
'use strict';
2+
3+
require('../common');
4+
const async_wrap = process.binding('async_wrap');
5+
const assert = require('assert');
6+
const crypto = require('crypto');
7+
const domain = require('domain');
8+
const spawn = require('child_process').spawn;
9+
const callbacks = [ 'init', 'pre', 'post', 'destroy' ];
10+
const toCall = process.argv[2];
11+
var msgCalled = 0;
12+
var msgReceived = 0;
13+
14+
function init() {
15+
if (toCall === 'init')
16+
throw new Error('init');
17+
}
18+
function pre() {
19+
if (toCall === 'pre')
20+
throw new Error('pre');
21+
}
22+
function post() {
23+
if (toCall === 'post')
24+
throw new Error('post');
25+
}
26+
function destroy() {
27+
if (toCall === 'destroy')
28+
throw new Error('destroy');
29+
}
30+
31+
if (typeof process.argv[2] === 'string') {
32+
async_wrap.setupHooks({ init, pre, post, destroy });
33+
async_wrap.enable();
34+
35+
process.on('uncaughtException', () => assert.ok(0, 'UNREACHABLE'));
36+
37+
const d = domain.create();
38+
d.on('error', () => assert.ok(0, 'UNREACHABLE'));
39+
d.run(() => {
40+
// Using randomBytes because timers are not yet supported.
41+
crypto.randomBytes(0, () => { });
42+
});
43+
44+
} else {
45+
46+
process.on('exit', (code) => {
47+
assert.equal(msgCalled, callbacks.length);
48+
assert.equal(msgCalled, msgReceived);
49+
});
50+
51+
callbacks.forEach((item) => {
52+
msgCalled++;
53+
54+
const child = spawn(process.execPath, [__filename, item]);
55+
var errstring = '';
56+
57+
child.stderr.on('data', (data) => {
58+
errstring += data.toString();
59+
});
60+
61+
child.on('close', (code) => {
62+
if (errstring.includes('Error: ' + item))
63+
msgReceived++;
64+
65+
assert.equal(code, 1, `${item} closed with code ${code}`);
66+
});
67+
});
68+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.