Skip to content

Commit 98967c9

Browse files
addaleaxjasnell
authored andcommitted
src: refactor async callback handling
- Merge the two almost-but-not-quite identical `MakeCallback()` implementations - Provide a public `CallbackScope` class for embedders in order to enable `MakeCallback()`-like behaviour without tying that to calling a JS function PR-URL: #14697 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent f60a2aa commit 98967c9

File tree

8 files changed

+256
-117
lines changed

8 files changed

+256
-117
lines changed

src/async-wrap.cc

Lines changed: 2 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -634,73 +634,8 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
634634
MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
635635
int argc,
636636
Local<Value>* argv) {
637-
CHECK(env()->context() == env()->isolate()->GetCurrentContext());
638-
639-
Environment::AsyncCallbackScope callback_scope(env());
640-
641-
Environment::AsyncHooks::ExecScope exec_scope(env(),
642-
get_id(),
643-
get_trigger_id());
644-
645-
// Return v8::Undefined() because returning an empty handle will cause
646-
// ToLocalChecked() to abort.
647-
if (env()->using_domains() && DomainEnter(env(), object())) {
648-
return Undefined(env()->isolate());
649-
}
650-
651-
// No need to check a return value because the application will exit if an
652-
// exception occurs.
653-
AsyncWrap::EmitBefore(env(), get_id());
654-
655-
MaybeLocal<Value> ret = cb->Call(env()->context(), object(), argc, argv);
656-
657-
if (ret.IsEmpty()) {
658-
return ret;
659-
}
660-
661-
AsyncWrap::EmitAfter(env(), get_id());
662-
663-
// Return v8::Undefined() because returning an empty handle will cause
664-
// ToLocalChecked() to abort.
665-
if (env()->using_domains() && DomainExit(env(), object())) {
666-
return Undefined(env()->isolate());
667-
}
668-
669-
exec_scope.Dispose();
670-
671-
if (callback_scope.in_makecallback()) {
672-
return ret;
673-
}
674-
675-
Environment::TickInfo* tick_info = env()->tick_info();
676-
677-
if (tick_info->length() == 0) {
678-
env()->isolate()->RunMicrotasks();
679-
}
680-
681-
// Make sure the stack unwound properly. If there are nested MakeCallback's
682-
// then it should return early and not reach this code.
683-
CHECK_EQ(env()->current_async_id(), 0);
684-
CHECK_EQ(env()->trigger_id(), 0);
685-
686-
Local<Object> process = env()->process_object();
687-
688-
if (tick_info->length() == 0) {
689-
tick_info->set_index(0);
690-
return ret;
691-
}
692-
693-
MaybeLocal<Value> rcheck =
694-
env()->tick_callback_function()->Call(env()->context(),
695-
process,
696-
0,
697-
nullptr);
698-
699-
// Make sure the stack unwound properly.
700-
CHECK_EQ(env()->current_async_id(), 0);
701-
CHECK_EQ(env()->trigger_id(), 0);
702-
703-
return rcheck.IsEmpty() ? MaybeLocal<Value>() : ret;
637+
async_context context { get_id(), get_trigger_id() };
638+
return InternalMakeCallback(env(), object(), cb, argc, argv, context);
704639
}
705640

706641

src/node.cc

Lines changed: 119 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ using v8::SealHandleScope;
158158
using v8::String;
159159
using v8::TryCatch;
160160
using v8::Uint32Array;
161+
using v8::Undefined;
161162
using v8::V8;
162163
using v8::Value;
163164

@@ -1344,85 +1345,153 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) {
13441345
env->AddPromiseHook(fn, arg);
13451346
}
13461347

1348+
class InternalCallbackScope {
1349+
public:
1350+
InternalCallbackScope(Environment* env,
1351+
Local<Object> object,
1352+
const async_context& asyncContext);
1353+
~InternalCallbackScope();
1354+
void Close();
1355+
1356+
inline bool Failed() const { return failed_; }
1357+
inline void MarkAsFailed() { failed_ = true; }
1358+
inline bool IsInnerMakeCallback() const {
1359+
return callback_scope_.in_makecallback();
1360+
}
1361+
1362+
private:
1363+
Environment* env_;
1364+
async_context async_context_;
1365+
v8::Local<v8::Object> object_;
1366+
Environment::AsyncCallbackScope callback_scope_;
1367+
bool failed_ = false;
1368+
bool pushed_ids_ = false;
1369+
bool closed_ = false;
1370+
};
1371+
1372+
CallbackScope::CallbackScope(Isolate* isolate,
1373+
Local<Object> object,
1374+
async_context asyncContext)
1375+
: private_(new InternalCallbackScope(Environment::GetCurrent(isolate),
1376+
object,
1377+
asyncContext)),
1378+
try_catch_(isolate) {
1379+
try_catch_.SetVerbose(true);
1380+
}
1381+
1382+
CallbackScope::~CallbackScope() {
1383+
if (try_catch_.HasCaught())
1384+
private_->MarkAsFailed();
1385+
delete private_;
1386+
}
1387+
1388+
InternalCallbackScope::InternalCallbackScope(Environment* env,
1389+
Local<Object> object,
1390+
const async_context& asyncContext)
1391+
: env_(env),
1392+
async_context_(asyncContext),
1393+
object_(object),
1394+
callback_scope_(env) {
1395+
CHECK(!object.IsEmpty());
13471396

1348-
MaybeLocal<Value> MakeCallback(Environment* env,
1349-
Local<Value> recv,
1350-
const Local<Function> callback,
1351-
int argc,
1352-
Local<Value> argv[],
1353-
async_context asyncContext) {
13541397
// If you hit this assertion, you forgot to enter the v8::Context first.
13551398
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
13561399

1357-
Local<Object> object;
1358-
1359-
Environment::AsyncCallbackScope callback_scope(env);
1360-
bool disposed_domain = false;
1361-
1362-
if (recv->IsObject()) {
1363-
object = recv.As<Object>();
1400+
if (env->using_domains()) {
1401+
failed_ = DomainEnter(env, object_);
1402+
if (failed_)
1403+
return;
13641404
}
13651405

1366-
if (env->using_domains()) {
1367-
CHECK(recv->IsObject());
1368-
disposed_domain = DomainEnter(env, object);
1369-
if (disposed_domain) return Undefined(env->isolate());
1406+
if (asyncContext.async_id != 0) {
1407+
// No need to check a return value because the application will exit if
1408+
// an exception occurs.
1409+
AsyncWrap::EmitBefore(env, asyncContext.async_id);
13701410
}
13711411

1372-
MaybeLocal<Value> ret;
1412+
env->async_hooks()->push_ids(async_context_.async_id,
1413+
async_context_.trigger_async_id);
1414+
pushed_ids_ = true;
1415+
}
13731416

1374-
{
1375-
AsyncHooks::ExecScope exec_scope(env, asyncContext.async_id,
1376-
asyncContext.trigger_async_id);
1417+
InternalCallbackScope::~InternalCallbackScope() {
1418+
Close();
1419+
}
13771420

1378-
if (asyncContext.async_id != 0) {
1379-
// No need to check a return value because the application will exit if
1380-
// an exception occurs.
1381-
AsyncWrap::EmitBefore(env, asyncContext.async_id);
1382-
}
1421+
void InternalCallbackScope::Close() {
1422+
if (closed_) return;
1423+
closed_ = true;
13831424

1384-
ret = callback->Call(env->context(), recv, argc, argv);
1425+
if (pushed_ids_)
1426+
env_->async_hooks()->pop_ids(async_context_.async_id);
13851427

1386-
if (ret.IsEmpty()) {
1387-
// NOTE: For backwards compatibility with public API we return Undefined()
1388-
// if the top level call threw.
1389-
return callback_scope.in_makecallback() ?
1390-
ret : Undefined(env->isolate());
1391-
}
1428+
if (failed_) return;
13921429

1393-
if (asyncContext.async_id != 0) {
1394-
AsyncWrap::EmitAfter(env, asyncContext.async_id);
1395-
}
1430+
if (async_context_.async_id != 0) {
1431+
AsyncWrap::EmitAfter(env_, async_context_.async_id);
13961432
}
13971433

1398-
if (env->using_domains()) {
1399-
disposed_domain = DomainExit(env, object);
1400-
if (disposed_domain) return Undefined(env->isolate());
1434+
if (env_->using_domains()) {
1435+
failed_ = DomainExit(env_, object_);
1436+
if (failed_) return;
14011437
}
14021438

1403-
if (callback_scope.in_makecallback()) {
1404-
return ret;
1439+
if (IsInnerMakeCallback()) {
1440+
return;
14051441
}
14061442

1407-
Environment::TickInfo* tick_info = env->tick_info();
1443+
Environment::TickInfo* tick_info = env_->tick_info();
14081444

14091445
if (tick_info->length() == 0) {
1410-
env->isolate()->RunMicrotasks();
1446+
env_->isolate()->RunMicrotasks();
14111447
}
14121448

14131449
// Make sure the stack unwound properly. If there are nested MakeCallback's
14141450
// then it should return early and not reach this code.
1415-
CHECK_EQ(env->current_async_id(), asyncContext.async_id);
1416-
CHECK_EQ(env->trigger_id(), asyncContext.trigger_async_id);
1451+
CHECK_EQ(env_->current_async_id(), 0);
1452+
CHECK_EQ(env_->trigger_id(), 0);
14171453

1418-
Local<Object> process = env->process_object();
1454+
Local<Object> process = env_->process_object();
14191455

14201456
if (tick_info->length() == 0) {
14211457
tick_info->set_index(0);
1422-
return ret;
1458+
return;
1459+
}
1460+
1461+
CHECK_EQ(env_->current_async_id(), 0);
1462+
CHECK_EQ(env_->trigger_id(), 0);
1463+
1464+
if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
1465+
failed_ = true;
1466+
}
1467+
}
1468+
1469+
MaybeLocal<Value> InternalMakeCallback(Environment* env,
1470+
Local<Object> recv,
1471+
const Local<Function> callback,
1472+
int argc,
1473+
Local<Value> argv[],
1474+
async_context asyncContext) {
1475+
InternalCallbackScope scope(env, recv, asyncContext);
1476+
if (scope.Failed()) {
1477+
return Undefined(env->isolate());
1478+
}
1479+
1480+
MaybeLocal<Value> ret;
1481+
1482+
{
1483+
ret = callback->Call(env->context(), recv, argc, argv);
1484+
1485+
if (ret.IsEmpty()) {
1486+
// NOTE: For backwards compatibility with public API we return Undefined()
1487+
// if the top level call threw.
1488+
scope.MarkAsFailed();
1489+
return scope.IsInnerMakeCallback() ? ret : Undefined(env->isolate());
1490+
}
14231491
}
14241492

1425-
if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
1493+
scope.Close();
1494+
if (scope.Failed()) {
14261495
return Undefined(env->isolate());
14271496
}
14281497

@@ -1475,8 +1544,8 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
14751544
// the two contexts need not be the same.
14761545
Environment* env = Environment::GetCurrent(callback->CreationContext());
14771546
Context::Scope context_scope(env->context());
1478-
return MakeCallback(env, recv.As<Value>(), callback, argc, argv,
1479-
asyncContext);
1547+
return InternalMakeCallback(env, recv, callback,
1548+
argc, argv, asyncContext);
14801549
}
14811550

14821551

src/node.h

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,36 @@ NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate,
599599
NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate,
600600
async_context asyncContext);
601601

602+
class InternalCallbackScope;
603+
604+
/* This class works like `MakeCallback()` in that it sets up a specific
605+
* asyncContext as the current one and informs the async_hooks and domains
606+
* modules that this context is currently active.
607+
*
608+
* `MakeCallback()` is a wrapper around this class as well as
609+
* `Function::Call()`. Either one of these mechanisms needs to be used for
610+
* top-level calls into JavaScript (i.e. without any existing JS stack).
611+
*
612+
* This object should be stack-allocated to ensure that it is contained in a
613+
* valid HandleScope.
614+
*/
615+
class NODE_EXTERN CallbackScope {
616+
public:
617+
CallbackScope(v8::Isolate* isolate,
618+
v8::Local<v8::Object> resource,
619+
async_context asyncContext);
620+
~CallbackScope();
621+
622+
private:
623+
InternalCallbackScope* private_;
624+
v8::TryCatch try_catch_;
625+
626+
void operator=(const CallbackScope&) = delete;
627+
void operator=(CallbackScope&&) = delete;
628+
CallbackScope(const CallbackScope&) = delete;
629+
CallbackScope(CallbackScope&&) = delete;
630+
};
631+
602632
/* An API specific to emit before/after callbacks is unnecessary because
603633
* MakeCallback will automatically call them for you.
604634
*
@@ -724,6 +754,16 @@ class AsyncResource {
724754
async_id get_trigger_async_id() const {
725755
return async_context_.trigger_async_id;
726756
}
757+
758+
protected:
759+
class CallbackScope : public node::CallbackScope {
760+
public:
761+
explicit CallbackScope(AsyncResource* res)
762+
: node::CallbackScope(res->isolate_,
763+
res->resource_.Get(res->isolate_),
764+
res->async_context_) {}
765+
};
766+
727767
private:
728768
v8::Isolate* isolate_;
729769
v8::Persistent<v8::Object> resource_;

src/node_internals.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,14 @@ static v8::MaybeLocal<v8::Object> New(Environment* env,
259259
}
260260
} // namespace Buffer
261261

262+
v8::MaybeLocal<v8::Value> InternalMakeCallback(
263+
Environment* env,
264+
v8::Local<v8::Object> recv,
265+
const v8::Local<v8::Function> callback,
266+
int argc,
267+
v8::Local<v8::Value> argv[],
268+
async_context asyncContext);
269+
262270
} // namespace node
263271

264272
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

0 commit comments

Comments
 (0)