Skip to content

Commit

Permalink
async_wrap: don't abort on callback exception
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
  • Loading branch information
trevnorris authored and evanlucas committed Mar 30, 2016
1 parent 6f16882 commit 2cbbaaf
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 12 deletions.
15 changes: 11 additions & 4 deletions src/async-wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,15 @@ inline AsyncWrap::AsyncWrap(Environment* env,
argv[3] = parent->object();
}

v8::TryCatch try_catch(env->isolate());

v8::MaybeLocal<v8::Value> ret =
init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv);

if (ret.IsEmpty())
FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");
if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
}

bits_ |= 1; // ran_init_callback() is true now.
}
Expand All @@ -69,10 +73,13 @@ inline AsyncWrap::~AsyncWrap() {
if (!fn.IsEmpty()) {
v8::HandleScope scope(env()->isolate());
v8::Local<v8::Value> uid = v8::Integer::New(env()->isolate(), get_uid());
v8::TryCatch try_catch(env()->isolate());
v8::MaybeLocal<v8::Value> ret =
fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid);
if (ret.IsEmpty())
FatalError("node::AsyncWrap::~AsyncWrap", "destroy hook threw");
if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env());
FatalException(env()->isolate(), try_catch);
}
}
}

Expand Down
20 changes: 16 additions & 4 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ using v8::HeapProfiler;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::MaybeLocal;
using v8::Object;
using v8::RetainedObjectInfo;
using v8::TryCatch;
Expand Down Expand Up @@ -225,17 +226,28 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}

if (ran_init_callback() && !pre_fn.IsEmpty()) {
if (pre_fn->Call(context, 1, &uid).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
TryCatch try_catch(env()->isolate());
MaybeLocal<Value> ar = pre_fn->Call(env()->context(), context, 1, &uid);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env());
FatalException(env()->isolate(), try_catch);
return Local<Value>();
}
}

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

if (ran_init_callback() && !post_fn.IsEmpty()) {
Local<Value> did_throw = Boolean::New(env()->isolate(), ret.IsEmpty());
Local<Value> vals[] = { uid, did_throw };
if (post_fn->Call(context, ARRAY_SIZE(vals), vals).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
TryCatch try_catch(env()->isolate());
MaybeLocal<Value> ar =
post_fn->Call(env()->context(), context, ARRAY_SIZE(vals), vals);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env());
FatalException(env()->isolate(), try_catch);
return Local<Value>();
}
}

if (ret.IsEmpty()) {
Expand Down
38 changes: 34 additions & 4 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1175,8 +1175,13 @@ Local<Value> MakeCallback(Environment* env,
}

if (ran_init_callback && !pre_fn.IsEmpty()) {
if (pre_fn->Call(object, 0, nullptr).IsEmpty())
FatalError("node::MakeCallback", "pre hook threw");
TryCatch try_catch(env->isolate());
MaybeLocal<Value> ar = pre_fn->Call(env->context(), object, 0, nullptr);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
return Local<Value>();
}
}

Local<Value> ret = callback->Call(recv, argc, argv);
Expand All @@ -1187,8 +1192,14 @@ Local<Value> MakeCallback(Environment* env,
// This needs to be fixed.
Local<Value> vals[] =
{ Undefined(env->isolate()).As<Value>(), did_throw };
if (post_fn->Call(object, ARRAY_SIZE(vals), vals).IsEmpty())
FatalError("node::MakeCallback", "post hook threw");
TryCatch try_catch(env->isolate());
MaybeLocal<Value> ar =
post_fn->Call(env->context(), object, ARRAY_SIZE(vals), vals);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
return Local<Value>();
}
}

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


void ClearFatalExceptionHandlers(Environment* env) {
Local<Object> process = env->process_object();
Local<Value> events =
process->Get(env->context(), env->events_string()).ToLocalChecked();

if (events->IsObject()) {
events.As<Object>()->Set(
env->context(),
OneByteString(env->isolate(), "uncaughtException"),
Undefined(env->isolate())).FromJust();
}

process->Set(
env->context(),
env->domain_string(),
Undefined(env->isolate())).FromJust();
}


static void Binding(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down
5 changes: 5 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,11 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
Environment* env_;
};

// Clear any domain and/or uncaughtException handlers to force the error's
// propagation and shutdown the process. Use this to force the process to exit
// by clearing all callbacks that could handle the error.
void ClearFatalExceptionHandlers(Environment* env);

enum NodeInstanceType { MAIN, WORKER };

class NodeInstanceData {
Expand Down
68 changes: 68 additions & 0 deletions test/parallel/test-async-wrap-throw-from-callback.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
'use strict';

require('../common');
const async_wrap = process.binding('async_wrap');
const assert = require('assert');
const crypto = require('crypto');
const domain = require('domain');
const spawn = require('child_process').spawn;
const callbacks = [ 'init', 'pre', 'post', 'destroy' ];
const toCall = process.argv[2];
var msgCalled = 0;
var msgReceived = 0;

function init() {
if (toCall === 'init')
throw new Error('init');
}
function pre() {
if (toCall === 'pre')
throw new Error('pre');
}
function post() {
if (toCall === 'post')
throw new Error('post');
}
function destroy() {
if (toCall === 'destroy')
throw new Error('destroy');
}

if (typeof process.argv[2] === 'string') {
async_wrap.setupHooks({ init, pre, post, destroy });
async_wrap.enable();

process.on('uncaughtException', () => assert.ok(0, 'UNREACHABLE'));

const d = domain.create();
d.on('error', () => assert.ok(0, 'UNREACHABLE'));
d.run(() => {
// Using randomBytes because timers are not yet supported.
crypto.randomBytes(0, () => { });
});

} else {

process.on('exit', (code) => {
assert.equal(msgCalled, callbacks.length);
assert.equal(msgCalled, msgReceived);
});

callbacks.forEach((item) => {
msgCalled++;

const child = spawn(process.execPath, [__filename, item]);
var errstring = '';

child.stderr.on('data', (data) => {
errstring += data.toString();
});

child.on('close', (code) => {
if (errstring.includes('Error: ' + item))
msgReceived++;

assert.equal(code, 1, `${item} closed with code ${code}`);
});
});
}

0 comments on commit 2cbbaaf

Please sign in to comment.