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: https://github.com/nodejs/node/pull/36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>

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,

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 {