Skip to content

Commit

Permalink
async_hooks: C++ Embedder API overhaul
Browse files Browse the repository at this point in the history 
* Fix AsyncHooksGetTriggerAsyncId such it corresponds to
async_hooks.triggerAsyncId and not async_hooks.initTriggerId.
* Use an async_context struct instead of two async_uid values.
  This change was necessary since the fixing
  AsyncHooksGetTriggerAsyncId otherwise makes it impossible to
  get the correct default trigger id. It also prevents an invalid
  triggerAsyncId in MakeCallback.
* Rename async_uid to async_id for consistency
* Rename get_uid to get_async_id
* Add get_trigger_async_id to AsyncResource class

PR-URL: nodejs#14040
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
@AndreasMadsen @addaleax
AndreasMadsen authored and addaleax committed Jul 11, 2017
1 parent f457b05 commit 3685f04
Show file tree
Hide file tree
Showing 10 changed files with 167 additions and 88 deletions.
39 changes: 25 additions & 14 deletions src/async-wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -741,40 +741,51 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
/* Public C++ embedder API */


async_uid AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
return Environment::GetCurrent(isolate)->current_async_id();
}

async_uid AsyncHooksGetCurrentId(Isolate* isolate) {
async_id AsyncHooksGetCurrentId(Isolate* isolate) {
return AsyncHooksGetExecutionAsyncId(isolate);
}


async_uid AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
return Environment::GetCurrent(isolate)->get_init_trigger_id();
async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
return Environment::GetCurrent(isolate)->trigger_id();
}

async_uid AsyncHooksGetTriggerId(Isolate* isolate) {
async_id AsyncHooksGetTriggerId(Isolate* isolate) {
return AsyncHooksGetTriggerAsyncId(isolate);
}


async_uid EmitAsyncInit(Isolate* isolate,
Local<Object> resource,
const char* name,
async_uid trigger_id) {
async_context EmitAsyncInit(Isolate* isolate,
Local<Object> resource,
const char* name,
async_id trigger_async_id) {
Environment* env = Environment::GetCurrent(isolate);
async_uid async_id = env->new_async_id();

// Initialize async context struct
if (trigger_async_id == -1)
trigger_async_id = env->get_init_trigger_id();

async_context context = {
env->new_async_id(), // async_id_
trigger_async_id // trigger_async_id_
};

// Run init hooks
Local<String> type =
String::NewFromUtf8(isolate, name, v8::NewStringType::kInternalized)
.ToLocalChecked();
AsyncWrap::EmitAsyncInit(env, resource, type, async_id, trigger_id);
return async_id;
AsyncWrap::EmitAsyncInit(env, resource, type, context.async_id,
context.trigger_async_id);

return context;
}

void EmitAsyncDestroy(Isolate* isolate, async_uid id) {
PushBackDestroyId(Environment::GetCurrent(isolate), id);
void EmitAsyncDestroy(Isolate* isolate, async_context asyncContext) {
PushBackDestroyId(Environment::GetCurrent(isolate), asyncContext.async_id);
}

} // namespace node
Expand Down
2 changes: 1 addition & 1 deletion src/inspector_agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ bool Agent::StartIoThread(bool wait_for_connect) {
message
};
MakeCallback(parent_env_->isolate(), process_object, emit_fn.As<Function>(),
arraysize(argv), argv, 0, 0);
arraysize(argv), argv, {0, 0});

return true;
}
Expand Down
49 changes: 23 additions & 26 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ static void CheckImmediate(uv_check_t* handle) {
env->immediate_callback_string(),
0,
nullptr,
0, 0).ToLocalChecked();
{0, 0}).ToLocalChecked();
}


Expand Down Expand Up @@ -1298,8 +1298,7 @@ MaybeLocal<Value> MakeCallback(Environment* env,
const Local<Function> callback,
int argc,
Local<Value> argv[],
double async_id,
double trigger_id) {
async_context asyncContext) {
// If you hit this assertion, you forgot to enter the v8::Context first.
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());

Expand All @@ -1321,10 +1320,12 @@ MaybeLocal<Value> MakeCallback(Environment* env,
MaybeLocal<Value> ret;

{
AsyncHooks::ExecScope exec_scope(env, async_id, trigger_id);
AsyncHooks::ExecScope exec_scope(env, asyncContext.async_id,
asyncContext.trigger_async_id);

if (async_id != 0) {
if (!AsyncWrap::EmitBefore(env, async_id)) return Local<Value>();
if (asyncContext.async_id != 0) {
if (!AsyncWrap::EmitBefore(env, asyncContext.async_id))
return Local<Value>();
}

ret = callback->Call(env->context(), recv, argc, argv);
Expand All @@ -1336,8 +1337,9 @@ MaybeLocal<Value> MakeCallback(Environment* env,
ret : Undefined(env->isolate());
}

if (async_id != 0) {
if (!AsyncWrap::EmitAfter(env, async_id)) return Local<Value>();
if (asyncContext.async_id != 0) {
if (!AsyncWrap::EmitAfter(env, asyncContext.async_id))
return Local<Value>();
}
}

Expand All @@ -1358,8 +1360,8 @@ MaybeLocal<Value> MakeCallback(Environment* env,

// Make sure the stack unwound properly. If there are nested MakeCallback's
// then it should return early and not reach this code.
CHECK_EQ(env->current_async_id(), async_id);
CHECK_EQ(env->trigger_id(), trigger_id);
CHECK_EQ(env->current_async_id(), asyncContext.async_id);
CHECK_EQ(env->trigger_id(), asyncContext.trigger_async_id);

Local<Object> process = env->process_object();

Expand All @@ -1384,13 +1386,11 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
const char* method,
int argc,
Local<Value> argv[],
async_uid async_id,
async_uid trigger_id) {
async_context asyncContext) {
Local<String> method_string =
String::NewFromUtf8(isolate, method, v8::NewStringType::kNormal)
.ToLocalChecked();
return MakeCallback(isolate, recv, method_string, argc, argv,
async_id, trigger_id);
return MakeCallback(isolate, recv, method_string, argc, argv, asyncContext);
}


Expand All @@ -1399,14 +1399,12 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
Local<String> symbol,
int argc,
Local<Value> argv[],
async_uid async_id,
async_uid trigger_id) {
async_context asyncContext) {
Local<Value> callback_v = recv->Get(symbol);
if (callback_v.IsEmpty()) return Local<Value>();
if (!callback_v->IsFunction()) return Local<Value>();
Local<Function> callback = callback_v.As<Function>();
return MakeCallback(isolate, recv, callback, argc, argv,
async_id, trigger_id);
return MakeCallback(isolate, recv, callback, argc, argv, asyncContext);
}


Expand All @@ -1415,8 +1413,7 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
Local<Function> callback,
int argc,
Local<Value> argv[],
async_uid async_id,
async_uid trigger_id) {
async_context asyncContext) {
// Observe the following two subtleties:
//
// 1. The environment is retrieved from the callback function's context.
Expand All @@ -1427,7 +1424,7 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
Environment* env = Environment::GetCurrent(callback->CreationContext());
Context::Scope context_scope(env->context());
return MakeCallback(env, recv.As<Value>(), callback, argc, argv,
async_id, trigger_id);
asyncContext);
}


Expand All @@ -1440,7 +1437,7 @@ Local<Value> MakeCallback(Isolate* isolate,
Local<Value>* argv) {
EscapableHandleScope handle_scope(isolate);
return handle_scope.Escape(
MakeCallback(isolate, recv, method, argc, argv, 0, 0)
MakeCallback(isolate, recv, method, argc, argv, {0, 0})
.FromMaybe(Local<Value>()));
}

Expand All @@ -1452,7 +1449,7 @@ Local<Value> MakeCallback(Isolate* isolate,
Local<Value>* argv) {
EscapableHandleScope handle_scope(isolate);
return handle_scope.Escape(
MakeCallback(isolate, recv, symbol, argc, argv, 0, 0)
MakeCallback(isolate, recv, symbol, argc, argv, {0, 0})
.FromMaybe(Local<Value>()));
}

Expand All @@ -1464,7 +1461,7 @@ Local<Value> MakeCallback(Isolate* isolate,
Local<Value>* argv) {
EscapableHandleScope handle_scope(isolate);
return handle_scope.Escape(
MakeCallback(isolate, recv, callback, argc, argv, 0, 0)
MakeCallback(isolate, recv, callback, argc, argv, {0, 0})
.FromMaybe(Local<Value>()));
}

Expand Down Expand Up @@ -4448,7 +4445,7 @@ void EmitBeforeExit(Environment* env) {
};
MakeCallback(env->isolate(),
process_object, "emit", arraysize(args), args,
0, 0).ToLocalChecked();
{0, 0}).ToLocalChecked();
}


Expand All @@ -4469,7 +4466,7 @@ int EmitExit(Environment* env) {

MakeCallback(env->isolate(),
process_object, "emit", arraysize(args), args,
0, 0).ToLocalChecked();
{0, 0}).ToLocalChecked();

// Reload exit code, it may be changed by `emit('exit')`
return process_object->Get(exitCode)->Int32Value();
Expand Down
72 changes: 40 additions & 32 deletions src/node.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ inline v8::Local<v8::Value> UVException(int errorno,
* These methods need to be called in a HandleScope.
*
* It is preferred that you use the `MakeCallback` overloads taking
* `async_uid` arguments.
* `async_id` arguments.
*/

NODE_EXTERN v8::Local<v8::Value> MakeCallback(
Expand Down Expand Up @@ -517,7 +517,11 @@ typedef void (*promise_hook_func) (v8::PromiseHookType type,
v8::Local<v8::Value> parent,
void* arg);

typedef double async_uid;
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. */
Expand All @@ -528,17 +532,17 @@ NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate,
/* Returns the id of the current execution context. If the return value is
* zero then no execution has been set. This will happen if the user handles
* I/O from native code. */
NODE_EXTERN async_uid AsyncHooksGetExecutionAsyncId(v8::Isolate* isolate);
NODE_EXTERN async_id AsyncHooksGetExecutionAsyncId(v8::Isolate* isolate);
/* legacy alias */
NODE_EXTERN NODE_DEPRECATED("Use AsyncHooksGetExecutionAsyncId(isolate)",
async_uid AsyncHooksGetCurrentId(v8::Isolate* isolate));
async_id AsyncHooksGetCurrentId(v8::Isolate* isolate));


/* Return same value as async_hooks.triggerAsyncId(); */
NODE_EXTERN async_uid AsyncHooksGetTriggerAsyncId(v8::Isolate* isolate);
NODE_EXTERN async_id AsyncHooksGetTriggerAsyncId(v8::Isolate* isolate);
/* legacy alias */
NODE_EXTERN NODE_DEPRECATED("Use AsyncHooksGetTriggerAsyncId(isolate)",
async_uid AsyncHooksGetTriggerId(v8::Isolate* isolate));
async_id AsyncHooksGetTriggerId(v8::Isolate* isolate));


/* If the native API doesn't inherit from the helper class then the callbacks
Expand All @@ -548,13 +552,14 @@ NODE_EXTERN NODE_DEPRECATED("Use AsyncHooksGetTriggerAsyncId(isolate)",
* The `trigger_async_id` parameter should correspond to the resource which is
* creating the new resource, which will usually be the return value of
* `AsyncHooksGetTriggerAsyncId()`. */
NODE_EXTERN async_uid EmitAsyncInit(v8::Isolate* isolate,
v8::Local<v8::Object> resource,
const char* name,
async_uid trigger_async_id);
NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate,
v8::Local<v8::Object> resource,
const char* name,
async_id trigger_async_id = -1);

/* Emit the destroy() callback. */
NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate, async_uid id);
NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate,
async_context asyncContext);

/* An API specific to emit before/after callbacks is unnecessary because
* MakeCallback will automatically call them for you.
Expand All @@ -572,24 +577,21 @@ v8::MaybeLocal<v8::Value> MakeCallback(v8::Isolate* isolate,
v8::Local<v8::Function> callback,
int argc,
v8::Local<v8::Value>* argv,
async_uid asyncId,
async_uid triggerAsyncId);
async_context asyncContext);
NODE_EXTERN
v8::MaybeLocal<v8::Value> MakeCallback(v8::Isolate* isolate,
v8::Local<v8::Object> recv,
const char* method,
int argc,
v8::Local<v8::Value>* argv,
async_uid asyncId,
async_uid triggerAsyncId);
async_context asyncContext);
NODE_EXTERN
v8::MaybeLocal<v8::Value> MakeCallback(v8::Isolate* isolate,
v8::Local<v8::Object> recv,
v8::Local<v8::String> symbol,
int argc,
v8::Local<v8::Value>* argv,
async_uid asyncId,
async_uid triggerAsyncId);
async_context asyncContext);

/* Helper class users can optionally inherit from. If
* `AsyncResource::MakeCallback()` is used, then all four callbacks will be
Expand All @@ -599,18 +601,15 @@ class AsyncResource {
AsyncResource(v8::Isolate* isolate,
v8::Local<v8::Object> resource,
const char* name,
async_uid trigger_async_id = -1)
async_id trigger_async_id = -1)
: isolate_(isolate),
resource_(isolate, resource),
trigger_async_id_(trigger_async_id) {
if (trigger_async_id_ == -1)
trigger_async_id_ = AsyncHooksGetTriggerAsyncId(isolate);

uid_ = EmitAsyncInit(isolate, resource, name, trigger_async_id_);
resource_(isolate, resource) {
async_context_ = EmitAsyncInit(isolate, resource, name,
trigger_async_id);
}

~AsyncResource() {
EmitAsyncDestroy(isolate_, uid_);
EmitAsyncDestroy(isolate_, async_context_);
}

v8::MaybeLocal<v8::Value> MakeCallback(
Expand All @@ -619,7 +618,7 @@ class AsyncResource {
v8::Local<v8::Value>* argv) {
return node::MakeCallback(isolate_, get_resource(),
callback, argc, argv,
uid_, trigger_async_id_);
async_context_);
}

v8::MaybeLocal<v8::Value> MakeCallback(
Expand All @@ -628,7 +627,7 @@ class AsyncResource {
v8::Local<v8::Value>* argv) {
return node::MakeCallback(isolate_, get_resource(),
method, argc, argv,
uid_, trigger_async_id_);
async_context_);
}

v8::MaybeLocal<v8::Value> MakeCallback(
Expand All @@ -637,21 +636,30 @@ class AsyncResource {
v8::Local<v8::Value>* argv) {
return node::MakeCallback(isolate_, get_resource(),
symbol, argc, argv,
uid_, trigger_async_id_);
async_context_);
}

v8::Local<v8::Object> get_resource() {
return resource_.Get(isolate_);
}

async_uid get_uid() const {
return uid_;
NODE_DEPRECATED("Use AsyncResource::get_async_id()",
async_id get_uid() const {
return get_async_id();
}
)

async_id get_async_id() const {
return async_context_.async_id;
}

async_id get_trigger_async_id() const {
return async_context_.trigger_async_id;
}
private:
v8::Isolate* isolate_;
v8::Persistent<v8::Object> resource_;
async_uid uid_;
async_uid trigger_async_id_;
async_context async_context_;
};

} // namespace node
Expand Down
Loading

0 comments on commit 3685f04

Please sign in to comment.