Skip to content

Commit

Permalink
src: introduce convenience node::MakeSyncCallback()
Browse files Browse the repository at this point in the history
There are situations where one wants to invoke a JS callback's ->Call()
from C++ and in particular retain any existing async_context state, but
where it's not obvious that a plain ->Call() would be safe at the point
in question.

Such callsites usually resort to
node::MakeCallback(..., async_context{0, 0}), which unconditionally
pushes the async_context{0, 0} and takes the required provisions for the
->Call() itself such as triggering the tick after its return, if needed.

An example would be the PerformanceObserver invocation from
PerformanceEntry::Notify(): this can get called when coming from JS
through e.g. perf_hooks.performance.mark() and alike, but perhaps also
from nghttp2 (c.f. EmitStatistics() in node_http2.cc).

In the former case, a plain ->Call() would be safe and it would be
desirable to retain the current async_context so that
PerformanceObservers can access it resp. the associated
AsyncLocalStorage. However, in the second case the additional provisions
taken by node::MakeCallback() might potentially be strictly required.

So PerformanceEntry::Notify() bites the bullet and invokes the
PerformanceObservers through node::MakeCallback() unconditionally,
thereby always rendering any possibly preexisting async_context
inaccessible.

Introduce the convenience node::MakeSyncCallback() for such usecases,
which would basically forward to ->Call() if safe and to
node::MakeCallback(..., async_context{0, 0}) otherwise.

Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>

PR-URL: #36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
nicstange authored and targos committed Dec 21, 2020
1 parent e597882 commit 4f3d7bb
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 0 deletions.
28 changes: 28 additions & 0 deletions src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,34 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
return ret;
}

// Use this if you just want to safely invoke some JS callback and
// would like to retain the currently active async_context, if any.
// In case none is available, a fixed default context will be
// installed otherwise.
MaybeLocal<Value> MakeSyncCallback(Isolate* isolate,
Local<Object> recv,
Local<Function> callback,
int argc,
Local<Value> argv[]) {
Environment* env = Environment::GetCurrent(callback->CreationContext());
CHECK_NOT_NULL(env);
if (!env->can_call_into_js()) return Local<Value>();

Context::Scope context_scope(env->context());
if (env->async_callback_scope_depth()) {
// There's another MakeCallback() on the stack, piggy back on it.
// In particular, retain the current async_context.
return callback->Call(env->context(), recv, argc, argv);
}

// This is a toplevel invocation and the caller (intentionally)
// didn't provide any async_context to run in. Install a default context.
MaybeLocal<Value> ret =
InternalMakeCallback(env, env->process_object(), recv, callback, argc, argv,
async_context{0, 0});
return ret;
}

// Legacy MakeCallback()s

Local<Value> MakeCallback(Isolate* isolate,
Expand Down
6 changes: 6 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,12 @@ v8::MaybeLocal<v8::Value> InternalMakeCallback(
v8::Local<v8::Value> argv[],
async_context asyncContext);

v8::MaybeLocal<v8::Value> MakeSyncCallback(v8::Isolate* isolate,
v8::Local<v8::Object> recv,
v8::Local<v8::Function> callback,
int argc,
v8::Local<v8::Value> argv[]);

class InternalCallbackScope {
public:
enum Flags {
Expand Down

0 comments on commit 4f3d7bb

Please sign in to comment.