Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

async_hooks: C++ Embedder API overhaul #14040

Closed
wants to merge 9 commits into from
Closed

async_hooks: C++ Embedder API overhaul #14040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4af3a33
async_hooks: C++ Embedder API overhaul
AndreasMadsen Jul 2, 2017
df876b8
[squash] use context struct and fix AsyncResources
AndreasMadsen Jul 2, 2017
1a6e24d
[squash] rename async_uid to async_id
AndreasMadsen Jul 2, 2017
c7c441e
[squash] minor cleanup
AndreasMadsen Jul 2, 2017
89e7ccd
[squash] cleanup struct def
AndreasMadsen Jul 2, 2017
e78d988
[squash] fix lint issue
AndreasMadsen Jul 3, 2017
9aa3ba2
[squash] make async_context properties public
AndreasMadsen Jul 3, 2017
98b5e9b
[squash] rename get_uid to get_async_id
AndreasMadsen Jul 5, 2017
48ef6a2
[squash] add AsyncResource::get_trigger_async_id
AndreasMadsen Jul 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions src/async-wrap.cc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 @@ -4425,7 +4422,7 @@ void EmitBeforeExit(Environment* env) {
};
MakeCallback(env->isolate(),
process_object, "emit", arraysize(args), args,
0, 0).ToLocalChecked();
{0, 0}).ToLocalChecked();
}


Expand All @@ -4446,7 +4443,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
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like this breakage would be shim-able… If that’s right, I’d say we label this dont-land and I can do a non-breaking backport to v8.x, if you like

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be awesome. In particular, I couldn't see how to make EmitAsyncInit backward compatible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old API required trigger_async_id instead of leaving it optional, so we could make it return a pair based on that parameter, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should EmitAsyncInit then return when trigger_async_id is specified?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t really understand the question, sorry. Basically, what we need to shim is the old definition

async_uid EmitAsyncInit(v8::Isolate* isolate,
                        v8::Local<v8::Object> resource,
                        const char* name,
                        async_uid trigger_async_id);

in terms of the new one in this PR, right?

So what’s speaking against OldStyleEmitAsyncInit(resource, name, trigger_async_id) := (NewStyleAsyncInit(resource, name, trigger_async_id)).async_id_?

Or are you thinking about the return type? The hacky solution would be to enable casting from async_context to async_id by returning async_id_ (and immediately deprecating that), it’s not pretty but as long as it doesn’t end up in master I’m okay with that.

(One more thing: I’m noticing, maybe we should not underscore-suffix the fields of async_context given that they are public, not internal…)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are you thinking about the return type? The hacky solution would be to enable casting from async_context to async_id by returning async_id_ (and immediately deprecating that), it’s not pretty but as long as it doesn’t end up in master I’m okay with that.

Yes, it is the return type. The casting makes sense.

(One more thing: I’m noticing, maybe we should not underscore-suffix the fields of async_context given that they are public, not internal…)

Right. Please check ffdd431d7d42efe939f3cc378814844b5796e4b5.


/* 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