Skip to content

Commit

Permalink
src: remove TryCatch in MakeCallback
Browse files Browse the repository at this point in the history
After attempting to use ReThrow() and Reset() there were cases where
firing the domain's error handlers was not happening. Or in some cases
reentering MakeCallback would still cause the domain enter callback to
abort (because the error had not been Reset yet).

In order for the script to properly stop execution when a subsequent
call to MakeCallback throws it must not be located within a TryCatch.

Ref: #7048
PR-URL: #4507
Reviewed-By: Fedor Indutny <fedor@indutny.com>
  • Loading branch information
trevnorris authored and Myles Borins committed Jul 12, 2016
1 parent 48b7b71 commit 981bbcd
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 41 deletions.
28 changes: 11 additions & 17 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -196,43 +196,39 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
}

TryCatch try_catch(env()->isolate());
try_catch.SetVerbose(true);

if (has_domain) {
Local<Value> enter_v = domain->Get(env()->enter_string());
if (enter_v->IsFunction()) {
enter_v.As<Function>()->Call(domain, 0, nullptr);
if (try_catch.HasCaught())
return Undefined(env()->isolate());
if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
FatalError("node::AsyncWrap::MakeCallback",
"domain enter callback threw, please report this");
}
}
}

if (ran_init_callback() && !pre_fn.IsEmpty()) {
pre_fn->Call(context, 0, nullptr);
if (try_catch.HasCaught())
if (pre_fn->Call(context, 0, nullptr).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
}

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

if (ran_init_callback() && !post_fn.IsEmpty()) {
post_fn->Call(context, 0, nullptr);
if (try_catch.HasCaught())
if (post_fn->Call(context, 0, nullptr).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
}

// If the return value is empty then the callback threw.
if (ret.IsEmpty()) {
return Undefined(env()->isolate());
}

if (has_domain) {
Local<Value> exit_v = domain->Get(env()->exit_string());
if (exit_v->IsFunction()) {
exit_v.As<Function>()->Call(domain, 0, nullptr);
if (try_catch.HasCaught())
return Undefined(env()->isolate());
if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
FatalError("node::AsyncWrap::MakeCallback",
"domain exit callback threw, please report this");
}
}
}

Expand All @@ -251,9 +247,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
return ret;
}

env()->tick_callback_function()->Call(process, 0, nullptr);

if (try_catch.HasCaught()) {
if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
return Undefined(env()->isolate());
}

Expand Down
13 changes: 4 additions & 9 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ using v8::Message;
using v8::StackFrame;
using v8::StackTrace;
using v8::TryCatch;
using v8::Value;

void Environment::PrintSyncTrace() const {
if (!trace_sync_io_)
Expand Down Expand Up @@ -73,16 +74,10 @@ bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) {
return true;
}

// process nextTicks after call
TryCatch try_catch(isolate());
try_catch.SetVerbose(true);
tick_callback_function()->Call(process_object(), 0, nullptr);
Local<Value> ret =
tick_callback_function()->Call(process_object(), 0, nullptr);

if (try_catch.HasCaught()) {
return false;
}

return true;
return !ret.IsEmpty();
}

} // namespace node
27 changes: 12 additions & 15 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1154,48 +1154,45 @@ Local<Value> MakeCallback(Environment* env,
}
}

TryCatch try_catch(env->isolate());
try_catch.SetVerbose(true);

if (has_domain) {
Local<Value> enter_v = domain->Get(env->enter_string());
if (enter_v->IsFunction()) {
enter_v.As<Function>()->Call(domain, 0, nullptr);
if (try_catch.HasCaught())
return Undefined(env->isolate());
if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
FatalError("node::MakeCallback",
"domain enter callback threw, please report this");
}
}
}

if (ran_init_callback && !pre_fn.IsEmpty()) {
pre_fn->Call(object, 0, nullptr);
if (try_catch.HasCaught())
if (pre_fn->Call(object, 0, nullptr).IsEmpty())
FatalError("node::MakeCallback", "pre hook threw");
}

Local<Value> ret = callback->Call(recv, argc, argv);

if (ran_init_callback && !post_fn.IsEmpty()) {
post_fn->Call(object, 0, nullptr);
if (try_catch.HasCaught())
if (post_fn->Call(object, 0, nullptr).IsEmpty())
FatalError("node::MakeCallback", "post hook threw");
}

// If the return value is empty then the callback threw.
if (ret.IsEmpty()) {
return Undefined(env->isolate());
}

if (has_domain) {
Local<Value> exit_v = domain->Get(env->exit_string());
if (exit_v->IsFunction()) {
exit_v.As<Function>()->Call(domain, 0, nullptr);
if (try_catch.HasCaught())
return Undefined(env->isolate());
if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
FatalError("node::MakeCallback",
"domain exit callback threw, please report this");
}
}
}

if (!env->KickNextTick(&callback_scope))
if (!env->KickNextTick(&callback_scope)) {
return Undefined(env->isolate());
}

return ret;
}
Expand Down

0 comments on commit 981bbcd

Please sign in to comment.