src: introduce convenience node::MakeSyncCallback()

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>
targos · Dec 22, 2020 · Dec 6, 2020 · Nov 24, 2020 · Nov 20, 2020 · Nov 20, 2020
commit 4f3d7bb4178ef5742cb6248a413992c3326c3055
diff --git a/src/api/callback.cc b/src/api/callback.cc
@@ -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,

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -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 {