Skip to content

Commit

Permalink
src: remove AddPromiseHook()
Browse files Browse the repository at this point in the history 
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: #26529

PR-URL: #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>
  • Loading branch information
@addaleax
addaleax committed Mar 21, 2019
1 parent 1500e5d commit 96c3224
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 113 deletions.
6 changes: 0 additions & 6 deletions src/api/hooks.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
26 changes: 17 additions & 9 deletions src/async_wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}


Expand Down
2 changes: 1 addition & 1 deletion src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
71 changes: 0 additions & 71 deletions src/env.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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);
Expand Down
13 changes: 0 additions & 13 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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);

Expand Down
12 changes: 0 additions & 12 deletions src/node.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
18 changes: 17 additions & 1 deletion src/node_internals.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 96c3224

Please sign in to comment.