From 9c837c696d170fdceccf2213d0956a83948bc222 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Mar 2019 22:03:15 +0000 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/26529 --- src/api/hooks.cc | 6 ---- src/async_wrap.cc | 26 ++++++++++------ src/env-inl.h | 2 +- src/env.cc | 71 -------------------------------------------- src/env.h | 13 -------- src/node.h | 12 -------- src/node_internals.h | 18 ++++++++++- 7 files changed, 35 insertions(+), 113 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 4663df43a659af..6a0319c61cc767 100644 --- a/src/api/hooks.cc +++ b/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) { diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 2087db2667c467..582744ff2d792e 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -227,8 +227,14 @@ static PromiseWrap* extractPromiseWrap(Local promise) { } static void PromiseHook(PromiseHookType type, Local promise, - Local parent, void* arg) { - Environment* env = static_cast(arg); + Local parent) { + Local 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& args) { static void EnablePromiseHook(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - env->AddPromiseHook(PromiseHook, static_cast(env)); + args.GetIsolate()->SetPromiseHook(PromiseHook); } static void DisablePromiseHook(const FunctionCallbackInfo& 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(data); - env->RemovePromiseHook(PromiseHook, data); - }, static_cast(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(data); + isolate->SetPromiseHook(nullptr); + }, static_cast(isolate)); } diff --git a/src/env-inl.h b/src/env-inl.h index 33c7868e247c7d..1197115318b5ed 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -286,7 +286,7 @@ inline void Environment::AssignToContext(v8::Local 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 diff --git a/src/env.cc b/src/env.cc index 768e14d796d5bf..f0dd9abeb62801 100644 --- a/src/env.cc +++ b/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( static_cast(&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, - Local parent) { - Local 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); diff --git a/src/env.h b/src/env.h index 70c566dce9a08f..4e928e6f31c339 100644 --- a/src/env.h +++ b/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 at_exit_functions_; - struct PromiseHookCallback { - promise_hook_func cb_; - void* arg_; - size_t enable_count_; - }; - std::vector 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 promise, - v8::Local parent); - template void ForEachBaseObject(T&& iterator); diff --git a/src/node.h b/src/node.h index 6568085335605d..32fd4b288e75f3 100644 --- a/src/node.h +++ b/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 promise, - v8::Local 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. diff --git a/src/node_internals.h b/src/node_internals.h index bc6a36d9db4a21..a6994099537116 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -301,7 +301,6 @@ v8::MaybeLocal GetPerContextExports(v8::Local 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