Skip to content

Commit 95afe28

Browse files
trevnorristrevnorris
committed
src: remove TryCatch in MakeCallback
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. PR-URL: nodejs#4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>
1 parent 5e30478 commit 95afe28

3 files changed

Lines changed: 27 additions & 41 deletions

File tree

‎src/async-wrap.cc‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -196,43 +196,39 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
196196
}
197197
}
198198

199-
TryCatch try_catch(env()->isolate());
200-
try_catch.SetVerbose(true);
201-
202199
if (has_domain) {
203200
Local<Value> enter_v = domain->Get(env()->enter_string());
204201
if (enter_v->IsFunction()) {
205-
enter_v.As<Function>()->Call(domain, 0, nullptr);
206-
if (try_catch.HasCaught())
207-
return Undefined(env()->isolate());
202+
if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
203+
FatalError("node::AsyncWrap::MakeCallback",
204+
"domain enter callback threw, please report this");
205+
}
208206
}
209207
}
210208

211209
if (ran_init_callback() && !pre_fn.IsEmpty()) {
212-
pre_fn->Call(context, 0, nullptr);
213-
if (try_catch.HasCaught())
210+
if (pre_fn->Call(context, 0, nullptr).IsEmpty())
214211
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
215212
}
216213

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

219216
if (ran_init_callback() && !post_fn.IsEmpty()) {
220-
post_fn->Call(context, 0, nullptr);
221-
if (try_catch.HasCaught())
217+
if (post_fn->Call(context, 0, nullptr).IsEmpty())
222218
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
223219
}
224220

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

230225
if (has_domain) {
231226
Local<Value> exit_v = domain->Get(env()->exit_string());
232227
if (exit_v->IsFunction()) {
233-
exit_v.As<Function>()->Call(domain, 0, nullptr);
234-
if (try_catch.HasCaught())
235-
return Undefined(env()->isolate());
228+
if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
229+
FatalError("node::AsyncWrap::MakeCallback",
230+
"domain exit callback threw, please report this");
231+
}
236232
}
237233
}
238234

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

254-
env()->tick_callback_function()->Call(process, 0, nullptr);
255-
256-
if (try_catch.HasCaught()) {
250+
if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
257251
return Undefined(env()->isolate());
258252
}
259253

‎src/env.cc‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ using v8::Message;
1818
using v8::StackFrame;
1919
using v8::StackTrace;
2020
using v8::TryCatch;
21+
using v8::Value;
2122

2223
void Environment::PrintSyncTrace() const {
2324
if (!trace_sync_io_)
@@ -80,16 +81,10 @@ bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) {
8081
return true;
8182
}
8283

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

88-
if (try_catch.HasCaught()) {
89-
return false;
90-
}
91-
92-
return true;
87+
return !ret.IsEmpty();
9388
}
9489

9590
} // namespace node

‎src/node.cc‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1162,48 +1162,45 @@ Local<Value> MakeCallback(Environment* env,
11621162
}
11631163
}
11641164

1165-
TryCatch try_catch(env->isolate());
1166-
try_catch.SetVerbose(true);
1167-
11681165
if (has_domain) {
11691166
Local<Value> enter_v = domain->Get(env->enter_string());
11701167
if (enter_v->IsFunction()) {
1171-
enter_v.As<Function>()->Call(domain, 0, nullptr);
1172-
if (try_catch.HasCaught())
1173-
return Undefined(env->isolate());
1168+
if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
1169+
FatalError("node::MakeCallback",
1170+
"domain enter callback threw, please report this");
1171+
}
11741172
}
11751173
}
11761174

11771175
if (ran_init_callback && !pre_fn.IsEmpty()) {
1178-
pre_fn->Call(object, 0, nullptr);
1179-
if (try_catch.HasCaught())
1176+
if (pre_fn->Call(object, 0, nullptr).IsEmpty())
11801177
FatalError("node::MakeCallback", "pre hook threw");
11811178
}
11821179

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

11851182
if (ran_init_callback && !post_fn.IsEmpty()) {
1186-
post_fn->Call(object, 0, nullptr);
1187-
if (try_catch.HasCaught())
1183+
if (post_fn->Call(object, 0, nullptr).IsEmpty())
11881184
FatalError("node::MakeCallback", "post hook threw");
11891185
}
11901186

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

11961191
if (has_domain) {
11971192
Local<Value> exit_v = domain->Get(env->exit_string());
11981193
if (exit_v->IsFunction()) {
1199-
exit_v.As<Function>()->Call(domain, 0, nullptr);
1200-
if (try_catch.HasCaught())
1201-
return Undefined(env->isolate());
1194+
if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
1195+
FatalError("node::MakeCallback",
1196+
"domain exit callback threw, please report this");
1197+
}
12021198
}
12031199
}
12041200

1205-
if (!env->KickNextTick(&callback_scope))
1201+
if (!env->KickNextTick(&callback_scope)) {
12061202
return Undefined(env->isolate());
1203+
}
12071204

12081205
return ret;
12091206
}

0 commit comments

Comments
 (0)