src: remove AddPromiseHook()

Remove this, as the underlying `Isolate::SetPromiseHook()` may be
removed as well in its current form in the future, and `async_hooks`
also serves this use case.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/
Refs: nodejs/node#26529

PR-URL: nodejs/node#26574
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Author: addaleax

src/api/hooks.cc

@@ -65,12 +65,6 @@ int EmitExit(Environment* env) {
       .ToChecked();
 }
 
-void AddPromiseHook(Isolate* isolate, promise_hook_func fn, void* arg) {
-  Environment* env = Environment::GetCurrent(isolate);
-  CHECK_NOT_NULL(env);
-  env->AddPromiseHook(fn, arg);
-}
-
 void AddEnvironmentCleanupHook(Isolate* isolate,
                                void (*fun)(void* arg),
                                void* arg) {

src/async_wrap.cc

@@ -227,8 +227,14 @@ static PromiseWrap* extractPromiseWrap(Local<Promise> promise) {
 }
 
 static void PromiseHook(PromiseHookType type, Local<Promise> promise,
-                        Local<Value> parent, void* arg) {
-  Environment* env = static_cast<Environment*>(arg);
+                        Local<Value> parent) {
+  Local<Context> context = promise->CreationContext();
+
+  Environment* env = Environment::GetCurrent(context);
+  if (env == nullptr) return;
+  TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
+                              "EnvPromiseHook", env);
+
   PromiseWrap* wrap = extractPromiseWrap(promise);
   if (type == PromiseHookType::kInit || wrap == nullptr) {
     bool silent = type != PromiseHookType::kInit;
@@ -318,20 +324,22 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
 
 
 static void EnablePromiseHook(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-  env->AddPromiseHook(PromiseHook, static_cast<void*>(env));
+  args.GetIsolate()->SetPromiseHook(PromiseHook);
 }
 
 
 static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
+  Isolate* isolate = args.GetIsolate();
 
   // Delay the call to `RemovePromiseHook` because we might currently be
   // between the `before` and `after` calls of a Promise.
-  env->isolate()->EnqueueMicrotask([](void* data) {
-    Environment* env = static_cast<Environment*>(data);
-    env->RemovePromiseHook(PromiseHook, data);
-  }, static_cast<void*>(env));
+  isolate->EnqueueMicrotask([](void* data) {
+    // The per-Isolate API provides no way of knowing whether there are multiple
+    // users of the PromiseHook. That hopefully goes away when V8 introduces
+    // a per-context API.
+    Isolate* isolate = static_cast<Isolate*>(data);
+    isolate->SetPromiseHook(nullptr);
+  }, static_cast<void*>(isolate));
 }
 
 

src/env-inl.h

@@ -286,7 +286,7 @@ inline void Environment::AssignToContext(v8::Local<v8::Context> context,
                                          const ContextInfo& info) {
   context->SetAlignedPointerInEmbedderData(
       ContextEmbedderIndex::kEnvironment, this);
-  // Used by EnvPromiseHook to know that we are on a node context.
+  // Used by Environment::GetCurrent to know that we are on a node context.
   context->SetAlignedPointerInEmbedderData(
     ContextEmbedderIndex::kContextTag, Environment::kNodeContextTagPtr);
 #if HAVE_INSPECTOR

src/env.cc

@@ -39,8 +39,6 @@ using v8::NewStringType;
 using v8::Number;
 using v8::Object;
 using v8::Private;
-using v8::Promise;
-using v8::PromiseHookType;
 using v8::StackFrame;
 using v8::StackTrace;
 using v8::String;
@@ -50,25 +48,6 @@ using v8::Undefined;
 using v8::Value;
 using worker::Worker;
 
-// TODO(@jasnell): Likely useful to move this to util or node_internal to
-// allow reuse. But since we're not reusing it yet...
-class TraceEventScope {
- public:
-  TraceEventScope(const char* category,
-                  const char* name,
-                  void* id) : category_(category), name_(name), id_(id) {
-    TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(category_, name_, id_);
-  }
-  ~TraceEventScope() {
-    TRACE_EVENT_NESTABLE_ASYNC_END0(category_, name_, id_);
-  }
-
- private:
-  const char* category_;
-  const char* name_;
-  void* id_;
-};
-
 int const Environment::kNodeContextTag = 0x6e6f64;
 void* const Environment::kNodeContextTagPtr = const_cast<void*>(
     static_cast<const void*>(&Environment::kNodeContextTag));
@@ -590,56 +569,6 @@ void Environment::AtExit(void (*cb)(void* arg), void* arg) {
   at_exit_functions_.push_back(ExitCallback{cb, arg});
 }
 
-void Environment::AddPromiseHook(promise_hook_func fn, void* arg) {
-  auto it = std::find_if(
-      promise_hooks_.begin(), promise_hooks_.end(),
-      [&](const PromiseHookCallback& hook) {
-        return hook.cb_ == fn && hook.arg_ == arg;
-      });
-  if (it != promise_hooks_.end()) {
-    it->enable_count_++;
-    return;
-  }
-  promise_hooks_.push_back(PromiseHookCallback{fn, arg, 1});
-
-  if (promise_hooks_.size() == 1) {
-    isolate_->SetPromiseHook(EnvPromiseHook);
-  }
-}
-
-bool Environment::RemovePromiseHook(promise_hook_func fn, void* arg) {
-  auto it = std::find_if(
-      promise_hooks_.begin(), promise_hooks_.end(),
-      [&](const PromiseHookCallback& hook) {
-        return hook.cb_ == fn && hook.arg_ == arg;
-      });
-
-  if (it == promise_hooks_.end()) return false;
-
-  if (--it->enable_count_ > 0) return true;
-
-  promise_hooks_.erase(it);
-  if (promise_hooks_.empty()) {
-    isolate_->SetPromiseHook(nullptr);
-  }
-
-  return true;
-}
-
-void Environment::EnvPromiseHook(PromiseHookType type,
-                                 Local<Promise> promise,
-                                 Local<Value> parent) {
-  Local<Context> context = promise->CreationContext();
-
-  Environment* env = Environment::GetCurrent(context);
-  if (env == nullptr) return;
-  TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
-                              "EnvPromiseHook", env);
-  for (const PromiseHookCallback& hook : env->promise_hooks_) {
-    hook.cb_(type, promise, parent, hook.arg_);
-  }
-}
-
 void Environment::RunAndClearNativeImmediates() {
   TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
                               "RunAndClearNativeImmediates", this);

src/env.h

@@ -959,8 +959,6 @@ class Environment {
   inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
   inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }
 
-  void AddPromiseHook(promise_hook_func fn, void* arg);
-  bool RemovePromiseHook(promise_hook_func fn, void* arg);
   inline bool EmitProcessEnvWarning() {
     bool current_value = emit_env_nonstring_warning_;
     emit_env_nonstring_warning_ = false;
@@ -1164,13 +1162,6 @@ class Environment {
 
   std::list<ExitCallback> at_exit_functions_;
 
-  struct PromiseHookCallback {
-    promise_hook_func cb_;
-    void* arg_;
-    size_t enable_count_;
-  };
-  std::vector<PromiseHookCallback> promise_hooks_;
-
   struct NativeImmediateCallback {
     native_immediate_callback cb_;
     void* data_;
@@ -1214,10 +1205,6 @@ class Environment {
   // Used by embedders to shutdown running Node instance.
   AsyncRequest thread_stopper_;
 
-  static void EnvPromiseHook(v8::PromiseHookType type,
-                             v8::Local<v8::Promise> promise,
-                             v8::Local<v8::Value> parent);
-
   template <typename T>
   void ForEachBaseObject(T&& iterator);
 

src/node.h

@@ -639,24 +639,12 @@ NODE_EXTERN void AtExit(Environment* env,
                         void (*cb)(void* arg),
                         void* arg = nullptr);
 
-typedef void (*promise_hook_func) (v8::PromiseHookType type,
-                                   v8::Local<v8::Promise> promise,
-                                   v8::Local<v8::Value> parent,
-                                   void* arg);
-
 typedef double async_id;
 struct async_context {
   ::node::async_id async_id;
   ::node::async_id trigger_async_id;
 };
 
-/* Registers an additional v8::PromiseHook wrapper. This API exists because V8
- * itself supports only a single PromiseHook. */
-NODE_DEPRECATED("Use async_hooks directly instead",
-                NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate,
-                                                promise_hook_func fn,
-                                                void* arg));
-
 /* This is a lot like node::AtExit, except that the hooks added via this
  * function are run before the AtExit ones and will always be registered
  * for the current Environment instance.

src/node_internals.h

@@ -301,7 +301,6 @@ v8::MaybeLocal<v8::Object> GetPerContextExports(v8::Local<v8::Context> context);
 namespace profiler {
 void StartCoverageCollection(Environment* env);
 }
-
 #ifdef _WIN32
 typedef SYSTEMTIME TIME_TYPE;
 #else  // UNIX, OSX
@@ -336,6 +335,23 @@ class DiagnosticFilename {
   std::string filename_;
 };
 
+class TraceEventScope {
+ public:
+  TraceEventScope(const char* category,
+                  const char* name,
+                  void* id) : category_(category), name_(name), id_(id) {
+    TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(category_, name_, id_);
+  }
+  ~TraceEventScope() {
+    TRACE_EVENT_NESTABLE_ASYNC_END0(category_, name_, id_);
+  }
+
+ private:
+  const char* category_;
+  const char* name_;
+  void* id_;
+};
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS