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 747f107

Browse filesBrowse files
trevnorrisMyles Borins
trevnorris
authored and
Myles Borins
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) Ref: #7048 PR-URL: #5756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
1 parent c06e2b0 commit 747f107
Copy full SHA for 747f107

File tree

Expand file treeCollapse file tree

5 files changed

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

5 files changed

+135
-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, arraysize(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, arraysize(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, arraysize(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
+35-4Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ using v8::Integer;
115115
using v8::Isolate;
116116
using v8::Local;
117117
using v8::Locker;
118+
using v8::MaybeLocal;
118119
using v8::Message;
119120
using v8::Number;
120121
using v8::Object;
@@ -1165,8 +1166,13 @@ Local<Value> MakeCallback(Environment* env,
11651166
}
11661167

11671168
if (ran_init_callback && !pre_fn.IsEmpty()) {
1168-
if (pre_fn->Call(object, 0, nullptr).IsEmpty())
1169-
FatalError("node::MakeCallback", "pre hook threw");
1169+
TryCatch try_catch(env->isolate());
1170+
MaybeLocal<Value> ar = pre_fn->Call(env->context(), object, 0, nullptr);
1171+
if (ar.IsEmpty()) {
1172+
ClearFatalExceptionHandlers(env);
1173+
FatalException(env->isolate(), try_catch);
1174+
return Local<Value>();
1175+
}
11701176
}
11711177

11721178
Local<Value> ret = callback->Call(recv, argc, argv);
@@ -1177,8 +1183,14 @@ Local<Value> MakeCallback(Environment* env,
11771183
// This needs to be fixed.
11781184
Local<Value> vals[] =
11791185
{ Undefined(env->isolate()).As<Value>(), did_throw };
1180-
if (post_fn->Call(object, arraysize(vals), vals).IsEmpty())
1181-
FatalError("node::MakeCallback", "post hook threw");
1186+
TryCatch try_catch(env->isolate());
1187+
MaybeLocal<Value> ar =
1188+
post_fn->Call(env->context(), object, arraysize(vals), vals);
1189+
if (ar.IsEmpty()) {
1190+
ClearFatalExceptionHandlers(env);
1191+
FatalException(env->isolate(), try_catch);
1192+
return Local<Value>();
1193+
}
11821194
}
11831195

11841196
if (ret.IsEmpty()) {
@@ -2352,6 +2364,25 @@ void OnMessage(Local<Message> message, Local<Value> error) {
23522364
}
23532365

23542366

2367+
void ClearFatalExceptionHandlers(Environment* env) {
2368+
Local<Object> process = env->process_object();
2369+
Local<Value> events =
2370+
process->Get(env->context(), env->events_string()).ToLocalChecked();
2371+
2372+
if (events->IsObject()) {
2373+
events.As<Object>()->Set(
2374+
env->context(),
2375+
OneByteString(env->isolate(), "uncaughtException"),
2376+
Undefined(env->isolate())).FromJust();
2377+
}
2378+
2379+
process->Set(
2380+
env->context(),
2381+
env->domain_string(),
2382+
Undefined(env->isolate())).FromJust();
2383+
}
2384+
2385+
23552386
static void Binding(const FunctionCallbackInfo<Value>& args) {
23562387
Environment* env = Environment::GetCurrent(args);
23572388

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
@@ -237,6 +237,11 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
237237
Environment* env_;
238238
};
239239

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

242247
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.