Skip to content

Commit 64616bb

Browse files
addaleaxaddaleax
committed
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: nodejs#14697 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent be6d807 commit 64616bb

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
@@ -657,73 +657,8 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
657657
MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
658658
int argc,
659659
Local<Value>* argv) {
660-
CHECK(env()->context() == env()->isolate()->GetCurrentContext());
661-
662-
Environment::AsyncCallbackScope callback_scope(env());
663-
664-
Environment::AsyncHooks::ExecScope exec_scope(env(),
665-
get_id(),
666-
get_trigger_id());
667-
668-
// Return v8::Undefined() because returning an empty handle will cause
669-
// ToLocalChecked() to abort.
670-
if (env()->using_domains() && DomainEnter(env(), object())) {
671-
return Undefined(env()->isolate());
672-
}
673-
674-
// No need to check a return value because the application will exit if an
675-
// exception occurs.
676-
AsyncWrap::EmitBefore(env(), get_id());
677-
678-
MaybeLocal<Value> ret = cb->Call(env()->context(), object(), argc, argv);
679-
680-
if (ret.IsEmpty()) {
681-
return ret;
682-
}
683-
684-
AsyncWrap::EmitAfter(env(), get_id());
685-
686-
// Return v8::Undefined() because returning an empty handle will cause
687-
// ToLocalChecked() to abort.
688-
if (env()->using_domains() && DomainExit(env(), object())) {
689-
return Undefined(env()->isolate());
690-
}
691-
692-
exec_scope.Dispose();
693-
694-
if (callback_scope.in_makecallback()) {
695-
return ret;
696-
}
697-
698-
Environment::TickInfo* tick_info = env()->tick_info();
699-
700-
if (tick_info->length() == 0) {
701-
env()->isolate()->RunMicrotasks();
702-
}
703-
704-
// Make sure the stack unwound properly. If there are nested MakeCallback's
705-
// then it should return early and not reach this code.
706-
CHECK_EQ(env()->current_async_id(), 0);
707-
CHECK_EQ(env()->trigger_id(), 0);
708-
709-
Local<Object> process = env()->process_object();
710-
711-
if (tick_info->length() == 0) {
712-
tick_info->set_index(0);
713-
return ret;
714-
}
715-
716-
MaybeLocal<Value> rcheck =
717-
env()->tick_callback_function()->Call(env()->context(),
718-
process,
719-
0,
720-
nullptr);
721-
722-
// Make sure the stack unwound properly.
723-
CHECK_EQ(env()->current_async_id(), 0);
724-
CHECK_EQ(env()->trigger_id(), 0);
725-
726-
return rcheck.IsEmpty() ? MaybeLocal<Value>() : ret;
660+
async_context context { get_id(), get_trigger_id() };
661+
return InternalMakeCallback(env(), object(), cb, argc, argv, context);
727662
}
728663

729664

‎src/node.cc‎

Lines changed: 119 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,7 @@ using v8::SealHandleScope;
161161
using v8::String;
162162
using v8::TryCatch;
163163
using v8::Uint32Array;
164+
using v8::Undefined;
164165
using v8::V8;
165166
using v8::Value;
166167

@@ -1311,85 +1312,153 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) {
13111312
env->AddPromiseHook(fn, arg);
13121313
}
13131314

1315+
class InternalCallbackScope {
1316+
public:
1317+
InternalCallbackScope(Environment* env,
1318+
Local<Object> object,
1319+
const async_context& asyncContext);
1320+
~InternalCallbackScope();
1321+
void Close();
1322+
1323+
inline bool Failed() const { return failed_; }
1324+
inline void MarkAsFailed() { failed_ = true; }
1325+
inline bool IsInnerMakeCallback() const {
1326+
return callback_scope_.in_makecallback();
1327+
}
1328+
1329+
private:
1330+
Environment* env_;
1331+
async_context async_context_;
1332+
v8::Local<v8::Object> object_;
1333+
Environment::AsyncCallbackScope callback_scope_;
1334+
bool failed_ = false;
1335+
bool pushed_ids_ = false;
1336+
bool closed_ = false;
1337+
};
13141338

1315-
MaybeLocal<Value> MakeCallback(Environment* env,
1316-
Local<Value> recv,
1317-
const Local<Function> callback,
1318-
int argc,
1319-
Local<Value> argv[],
1320-
async_context asyncContext) {
1321-
// If you hit this assertion, you forgot to enter the v8::Context first.
1322-
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
1339+
CallbackScope::CallbackScope(Isolate* isolate,
1340+
Local<Object> object,
1341+
async_context asyncContext)
1342+
: private_(new InternalCallbackScope(Environment::GetCurrent(isolate),
1343+
object,
1344+
asyncContext)),
1345+
try_catch_(isolate) {
1346+
try_catch_.SetVerbose(true);
1347+
}
13231348

1324-
Local<Object> object;
1349+
CallbackScope::~CallbackScope() {
1350+
if (try_catch_.HasCaught())
1351+
private_->MarkAsFailed();
1352+
delete private_;
1353+
}
13251354

1326-
Environment::AsyncCallbackScope callback_scope(env);
1327-
bool disposed_domain = false;
1355+
InternalCallbackScope::InternalCallbackScope(Environment* env,
1356+
Local<Object> object,
1357+
const async_context& asyncContext)
1358+
: env_(env),
1359+
async_context_(asyncContext),
1360+
object_(object),
1361+
callback_scope_(env) {
1362+
CHECK(!object.IsEmpty());
13281363

1329-
if (recv->IsObject()) {
1330-
object = recv.As<Object>();
1331-
}
1364+
// If you hit this assertion, you forgot to enter the v8::Context first.
1365+
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
13321366

13331367
if (env->using_domains()) {
1334-
CHECK(recv->IsObject());
1335-
disposed_domain = DomainEnter(env, object);
1336-
if (disposed_domain) return Undefined(env->isolate());
1368+
failed_ = DomainEnter(env, object_);
1369+
if (failed_)
1370+
return;
13371371
}
13381372

1339-
MaybeLocal<Value> ret;
1373+
if (asyncContext.async_id != 0) {
1374+
// No need to check a return value because the application will exit if
1375+
// an exception occurs.
1376+
AsyncWrap::EmitBefore(env, asyncContext.async_id);
1377+
}
13401378

1341-
{
1342-
AsyncHooks::ExecScope exec_scope(env, asyncContext.async_id,
1343-
asyncContext.trigger_async_id);
1379+
env->async_hooks()->push_ids(async_context_.async_id,
1380+
async_context_.trigger_async_id);
1381+
pushed_ids_ = true;
1382+
}
13441383

1345-
if (asyncContext.async_id != 0) {
1346-
// No need to check a return value because the application will exit if
1347-
// an exception occurs.
1348-
AsyncWrap::EmitBefore(env, asyncContext.async_id);
1349-
}
1384+
InternalCallbackScope::~InternalCallbackScope() {
1385+
Close();
1386+
}
13501387

1351-
ret = callback->Call(env->context(), recv, argc, argv);
1388+
void InternalCallbackScope::Close() {
1389+
if (closed_) return;
1390+
closed_ = true;
13521391

1353-
if (ret.IsEmpty()) {
1354-
// NOTE: For backwards compatibility with public API we return Undefined()
1355-
// if the top level call threw.
1356-
return callback_scope.in_makecallback() ?
1357-
ret : Undefined(env->isolate());
1358-
}
1392+
if (pushed_ids_)
1393+
env_->async_hooks()->pop_ids(async_context_.async_id);
13591394

1360-
if (asyncContext.async_id != 0) {
1361-
AsyncWrap::EmitAfter(env, asyncContext.async_id);
1362-
}
1395+
if (failed_) return;
1396+
1397+
if (async_context_.async_id != 0) {
1398+
AsyncWrap::EmitAfter(env_, async_context_.async_id);
13631399
}
13641400

1365-
if (env->using_domains()) {
1366-
disposed_domain = DomainExit(env, object);
1367-
if (disposed_domain) return Undefined(env->isolate());
1401+
if (env_->using_domains()) {
1402+
failed_ = DomainExit(env_, object_);
1403+
if (failed_) return;
13681404
}
13691405

1370-
if (callback_scope.in_makecallback()) {
1371-
return ret;
1406+
if (IsInnerMakeCallback()) {
1407+
return;
13721408
}
13731409

1374-
Environment::TickInfo* tick_info = env->tick_info();
1410+
Environment::TickInfo* tick_info = env_->tick_info();
13751411

13761412
if (tick_info->length() == 0) {
1377-
env->isolate()->RunMicrotasks();
1413+
env_->isolate()->RunMicrotasks();
13781414
}
13791415

13801416
// Make sure the stack unwound properly. If there are nested MakeCallback's
13811417
// then it should return early and not reach this code.
1382-
CHECK_EQ(env->current_async_id(), asyncContext.async_id);
1383-
CHECK_EQ(env->trigger_id(), asyncContext.trigger_async_id);
1418+
CHECK_EQ(env_->current_async_id(), 0);
1419+
CHECK_EQ(env_->trigger_id(), 0);
13841420

1385-
Local<Object> process = env->process_object();
1421+
Local<Object> process = env_->process_object();
13861422

13871423
if (tick_info->length() == 0) {
13881424
tick_info->set_index(0);
1389-
return ret;
1425+
return;
1426+
}
1427+
1428+
CHECK_EQ(env_->current_async_id(), 0);
1429+
CHECK_EQ(env_->trigger_id(), 0);
1430+
1431+
if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
1432+
failed_ = true;
1433+
}
1434+
}
1435+
1436+
MaybeLocal<Value> InternalMakeCallback(Environment* env,
1437+
Local<Object> recv,
1438+
const Local<Function> callback,
1439+
int argc,
1440+
Local<Value> argv[],
1441+
async_context asyncContext) {
1442+
InternalCallbackScope scope(env, recv, asyncContext);
1443+
if (scope.Failed()) {
1444+
return Undefined(env->isolate());
1445+
}
1446+
1447+
MaybeLocal<Value> ret;
1448+
1449+
{
1450+
ret = callback->Call(env->context(), recv, argc, argv);
1451+
1452+
if (ret.IsEmpty()) {
1453+
// NOTE: For backwards compatibility with public API we return Undefined()
1454+
// if the top level call threw.
1455+
scope.MarkAsFailed();
1456+
return scope.IsInnerMakeCallback() ? ret : Undefined(env->isolate());
1457+
}
13901458
}
13911459

1392-
if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
1460+
scope.Close();
1461+
if (scope.Failed()) {
13931462
return Undefined(env->isolate());
13941463
}
13951464

@@ -1442,8 +1511,8 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
14421511
// the two contexts need not be the same.
14431512
Environment* env = Environment::GetCurrent(callback->CreationContext());
14441513
Context::Scope context_scope(env->context());
1445-
return MakeCallback(env, recv.As<Value>(), callback, argc, argv,
1446-
asyncContext);
1514+
return InternalMakeCallback(env, recv, callback,
1515+
argc, argv, asyncContext);
14471516
}
14481517

14491518

‎src/node.h‎

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

573+
class InternalCallbackScope;
574+
575+
/* This class works like `MakeCallback()` in that it sets up a specific
576+
* asyncContext as the current one and informs the async_hooks and domains
577+
* modules that this context is currently active.
578+
*
579+
* `MakeCallback()` is a wrapper around this class as well as
580+
* `Function::Call()`. Either one of these mechanisms needs to be used for
581+
* top-level calls into JavaScript (i.e. without any existing JS stack).
582+
*
583+
* This object should be stack-allocated to ensure that it is contained in a
584+
* valid HandleScope.
585+
*/
586+
class NODE_EXTERN CallbackScope {
587+
public:
588+
CallbackScope(v8::Isolate* isolate,
589+
v8::Local<v8::Object> resource,
590+
async_context asyncContext);
591+
~CallbackScope();
592+
593+
private:
594+
InternalCallbackScope* private_;
595+
v8::TryCatch try_catch_;
596+
597+
void operator=(const CallbackScope&) = delete;
598+
void operator=(CallbackScope&&) = delete;
599+
CallbackScope(const CallbackScope&) = delete;
600+
CallbackScope(CallbackScope&&) = delete;
601+
};
602+
573603
/* An API specific to emit before/after callbacks is unnecessary because
574604
* MakeCallback will automatically call them for you.
575605
*
@@ -659,6 +689,16 @@ class AsyncResource {
659689
async_id get_trigger_async_id() const {
660690
return async_context_.trigger_async_id;
661691
}
692+
693+
protected:
694+
class CallbackScope : public node::CallbackScope {
695+
public:
696+
explicit CallbackScope(AsyncResource* res)
697+
: node::CallbackScope(res->isolate_,
698+
res->resource_.Get(res->isolate_),
699+
res->async_context_) {}
700+
};
701+
662702
private:
663703
v8::Isolate* isolate_;
664704
v8::Persistent<v8::Object> resource_;

‎src/node_internals.h‎

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

289+
v8::MaybeLocal<v8::Value> InternalMakeCallback(
290+
Environment* env,
291+
v8::Local<v8::Object> recv,
292+
const v8::Local<v8::Function> callback,
293+
int argc,
294+
v8::Local<v8::Value> argv[],
295+
async_context asyncContext);
296+
289297
} // namespace node
290298

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

0 commit comments

Comments
 (0)