From 09112ff2a57ae9dc9596b0a9358aedb659a5a83e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 May 2017 19:39:52 +0200 Subject: [PATCH 1/8] async_hooks: implement C++ embedder API --- src/async-wrap.cc | 123 ++++++++++++++++++------- src/async-wrap.h | 9 ++ src/inspector_agent.cc | 4 +- src/node.cc | 200 +++++++++++++++++++++++------------------ src/node.h | 121 +++++++++++++++++++++++-- src/node_crypto.cc | 6 +- src/node_internals.h | 21 ----- 7 files changed, 332 insertions(+), 152 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index d3776c73334b49..5a8afe390f7833 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -47,6 +47,7 @@ using v8::Object; using v8::Promise; using v8::PromiseHookType; using v8::RetainedObjectInfo; +using v8::String; using v8::Symbol; using v8::TryCatch; using v8::Uint32Array; @@ -216,23 +217,28 @@ bool DomainExit(Environment* env, v8::Local object) { static bool PreCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) { - AsyncHooks* async_hooks = wrap->env()->async_hooks(); - if (wrap->env()->using_domains() && run_domain_cbs) { bool is_disposed = DomainEnter(wrap->env(), wrap->object()); if (is_disposed) return false; } + return AsyncWrap::EmitBefore(wrap->env(), wrap->get_id()); +} + + +bool AsyncWrap::EmitBefore(Environment* env, double async_id) { + AsyncHooks* async_hooks = env->async_hooks(); + if (async_hooks->fields()[AsyncHooks::kBefore] > 0) { - Local uid = Number::New(wrap->env()->isolate(), wrap->get_id()); - Local fn = wrap->env()->async_hooks_before_function(); - TryCatch try_catch(wrap->env()->isolate()); + Local uid = Number::New(env->isolate(), async_id); + Local fn = env->async_hooks_before_function(); + TryCatch try_catch(env->isolate()); MaybeLocal ar = fn->Call( - wrap->env()->context(), Undefined(wrap->env()->isolate()), 1, &uid); + env->context(), Undefined(env->isolate()), 1, &uid); if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(wrap->env()); - FatalException(wrap->env()->isolate(), try_catch); + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); return false; } } @@ -242,29 +248,36 @@ static bool PreCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) { static bool PostCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) { - AsyncHooks* async_hooks = wrap->env()->async_hooks(); + if (!AsyncWrap::EmitAfter(wrap->env(), wrap->get_id())) + return false; + + if (wrap->env()->using_domains() && run_domain_cbs) { + bool is_disposed = DomainExit(wrap->env(), wrap->object()); + if (is_disposed) + return false; + } + + return true; +} + +bool AsyncWrap::EmitAfter(Environment* env, double async_id) { + AsyncHooks* async_hooks = env->async_hooks(); // If the callback failed then the after() hooks will be called at the end // of _fatalException(). if (async_hooks->fields()[AsyncHooks::kAfter] > 0) { - Local uid = Number::New(wrap->env()->isolate(), wrap->get_id()); - Local fn = wrap->env()->async_hooks_after_function(); - TryCatch try_catch(wrap->env()->isolate()); + Local uid = Number::New(env->isolate(), async_id); + Local fn = env->async_hooks_after_function(); + TryCatch try_catch(env->isolate()); MaybeLocal ar = fn->Call( - wrap->env()->context(), Undefined(wrap->env()->isolate()), 1, &uid); + env->context(), Undefined(env->isolate()), 1, &uid); if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(wrap->env()); - FatalException(wrap->env()->isolate(), try_catch); + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); return false; } } - if (wrap->env()->using_domains() && run_domain_cbs) { - bool is_disposed = DomainExit(wrap->env(), wrap->object()); - if (is_disposed) - return false; - } - return true; } @@ -526,32 +539,44 @@ AsyncWrap::~AsyncWrap() { // and reused over their lifetime. This way a new uid can be assigned when // the resource is pulled out of the pool and put back into use. void AsyncWrap::AsyncReset() { - AsyncHooks* async_hooks = env()->async_hooks(); async_id_ = env()->new_async_id(); trigger_id_ = env()->get_init_trigger_id(); + EmitAsyncInit(env(), object(), + env()->async_hooks()->provider_string(provider_type()), + async_id_, trigger_id_); +} + + +void AsyncWrap::EmitAsyncInit(Environment* env, + Local object, + Local type, + double async_id, + double trigger_id) { + AsyncHooks* async_hooks = env->async_hooks(); + // Nothing to execute, so can continue normally. if (async_hooks->fields()[AsyncHooks::kInit] == 0) { return; } - HandleScope scope(env()->isolate()); - Local init_fn = env()->async_hooks_init_function(); + HandleScope scope(env->isolate()); + Local init_fn = env->async_hooks_init_function(); Local argv[] = { - Number::New(env()->isolate(), get_id()), - env()->async_hooks()->provider_string(provider_type()), - object(), - Number::New(env()->isolate(), get_trigger_id()), + Number::New(env->isolate(), async_id), + type, + object, + Number::New(env->isolate(), trigger_id), }; - TryCatch try_catch(env()->isolate()); + TryCatch try_catch(env->isolate()); MaybeLocal ret = init_fn->Call( - env()->context(), object(), arraysize(argv), argv); + env->context(), object, arraysize(argv), argv); if (ret.IsEmpty()) { - ClearFatalExceptionHandlers(env()); - FatalException(env()->isolate(), try_catch); + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); } } @@ -620,6 +645,40 @@ Local AsyncWrap::MakeCallback(const Local cb, return rcheck.IsEmpty() ? Local() : ret_v; } + +/* Public C++ embedder API */ + + +async_uid AsyncHooksGetCurrentId(Isolate* isolate) { + return Environment::GetCurrent(isolate)->current_async_id(); +} + + +async_uid AsyncHooksGetTriggerId(Isolate* isolate) { + return Environment::GetCurrent(isolate)->get_init_trigger_id(); +} + + +async_uid EmitAsyncInit(Isolate* isolate, + Local resource, + const char* name, + async_uid trigger_id) { + Environment* env = Environment::GetCurrent(isolate); + async_uid async_id = env->new_async_id(); + if (trigger_id == -1) + trigger_id = env->get_init_trigger_id(); + + Local type = + String::NewFromUtf8(isolate, name, v8::NewStringType::kInternalized) + .ToLocalChecked(); + AsyncWrap::EmitAsyncInit(env, resource, type, async_id, trigger_id); + return async_id; +} + +void EmitAsyncDestroy(Isolate* isolate, async_uid id) { + PushBackDestroyId(Environment::GetCurrent(isolate), id); +} + } // namespace node NODE_MODULE_CONTEXT_AWARE_BUILTIN(async_wrap, node::AsyncWrap::Initialize) diff --git a/src/async-wrap.h b/src/async-wrap.h index cf81178747ae15..d3676a01c06705 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -101,6 +101,15 @@ class AsyncWrap : public BaseObject { static void AsyncReset(const v8::FunctionCallbackInfo& args); static void QueueDestroyId(const v8::FunctionCallbackInfo& args); + static void EmitAsyncInit(Environment* env, + v8::Local object, + v8::Local type, + double id, + double trigger_id); + + static bool EmitBefore(Environment* env, double id); + static bool EmitAfter(Environment* env, double id); + inline ProviderType provider_type() const; inline double get_id() const; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index a90edc5038fb2b..54a8957acf28d7 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -597,8 +597,8 @@ bool Agent::StartIoThread(bool wait_for_connect) { FIXED_ONE_BYTE_STRING(isolate, "internalMessage"), message }; - MakeCallback(parent_env_, process_object.As(), emit_fn.As(), - arraysize(argv), argv); + MakeCallback(parent_env_->isolate(), process_object, emit_fn.As(), + arraysize(argv), argv, 0); return true; } diff --git a/src/node.cc b/src/node.cc index 2c323be9f26da3..55315bd8f76b60 100644 --- a/src/node.cc +++ b/src/node.cc @@ -348,7 +348,12 @@ static void CheckImmediate(uv_check_t* handle) { Environment* env = Environment::from_immediate_check_handle(handle); HandleScope scope(env->isolate()); Context::Scope context_scope(env->context()); - MakeCallback(env, env->process_object(), env->immediate_callback_string()); + MakeCallback(env->isolate(), + env->process_object(), + env->immediate_callback_string(), + 0, + nullptr, + 0).ToLocalChecked(); } @@ -1281,18 +1286,20 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { } -Local MakeCallback(Environment* env, - Local recv, - const Local callback, - int argc, - Local argv[]) { +MaybeLocal MakeCallback(Environment* env, + Local recv, + const Local callback, + int argc, + Local argv[], + double async_id, + double trigger_id) { // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); - Local object, domain; - bool has_domain = false; + Local object; Environment::AsyncCallbackScope callback_scope(env); + bool disposed_domain = false; if (recv->IsObject()) { object = recv.As(); @@ -1300,51 +1307,40 @@ Local MakeCallback(Environment* env, if (env->using_domains()) { CHECK(recv->IsObject()); - Local domain_v = object->Get(env->domain_string()); - has_domain = domain_v->IsObject(); - if (has_domain) { - domain = domain_v.As(); - if (domain->Get(env->disposed_string())->IsTrue()) - return Undefined(env->isolate()); - } + disposed_domain = DomainEnter(env, object); + if (disposed_domain) return Undefined(env->isolate()); } - if (has_domain) { - Local enter_v = domain->Get(env->enter_string()); - if (enter_v->IsFunction()) { - if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { - FatalError("node::MakeCallback", - "domain enter callback threw, please report this"); - } - } - } + MaybeLocal ret; - // TODO(trevnorris): Correct this once node::MakeCallback() support id and - // triggerId. Consider completely removing it until then so the async id can - // propagate through to the fatalException after hook calls. - AsyncHooks::ExecScope exec_scope(env, 0, 0); + { + if (trigger_id == -1) + trigger_id = async_id; + AsyncHooks::ExecScope exec_scope(env, async_id, trigger_id); - Local ret = callback->Call(recv, argc, argv); + if (async_id != 0) { + if (!AsyncWrap::EmitBefore(env, async_id)) return Local(); + } - if (ret.IsEmpty()) { - // NOTE: For backwards compatibility with public API we return Undefined() - // if the top level call threw. - return callback_scope.in_makecallback() ? - ret : Undefined(env->isolate()).As(); - } + ret = callback->Call(env->context(), recv, argc, argv); - exec_scope.Dispose(); + if (ret.IsEmpty()) { + // NOTE: For backwards compatibility with public API we return Undefined() + // if the top level call threw. + return callback_scope.in_makecallback() ? + ret : Undefined(env->isolate()); + } - if (has_domain) { - Local exit_v = domain->Get(env->exit_string()); - if (exit_v->IsFunction()) { - if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { - FatalError("node::MakeCallback", - "domain exit callback threw, please report this"); - } + if (async_id != 0) { + if (!AsyncWrap::EmitAfter(env, async_id)) return Local(); } } + if (env->using_domains()) { + disposed_domain = DomainExit(env, object); + if (disposed_domain) return Undefined(env->isolate()); + } + if (callback_scope.in_makecallback()) { return ret; } @@ -1357,8 +1353,8 @@ Local 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(), 0); - CHECK_EQ(env->trigger_id(), 0); + CHECK_EQ(env->current_async_id(), async_id); + CHECK_EQ(env->trigger_id(), trigger_id); Local process = env->process_object(); @@ -1375,58 +1371,45 @@ Local MakeCallback(Environment* env, } -Local MakeCallback(Environment* env, - Local recv, - Local symbol, - int argc, - Local argv[]) { - Local cb_v = recv->Get(symbol); - CHECK(cb_v->IsFunction()); - return MakeCallback(env, recv.As(), cb_v.As(), argc, argv); -} - - -Local MakeCallback(Environment* env, - Local recv, - const char* method, - int argc, - Local argv[]) { - Local method_string = OneByteString(env->isolate(), method); - return MakeCallback(env, recv, method_string, argc, argv); -} +// Public MakeCallback()s -Local MakeCallback(Isolate* isolate, - Local recv, - const char* method, - int argc, - Local argv[]) { - EscapableHandleScope handle_scope(isolate); +MaybeLocal MakeCallback(Isolate* isolate, + Local recv, + const char* method, + int argc, + Local argv[], + async_uid async_id, + async_uid trigger_id) { Local method_string = OneByteString(isolate, method); - return handle_scope.Escape( - MakeCallback(isolate, recv, method_string, argc, argv)); + return MakeCallback(isolate, recv, method_string, argc, argv, + async_id, trigger_id); } -Local MakeCallback(Isolate* isolate, - Local recv, - Local symbol, - int argc, - Local argv[]) { - EscapableHandleScope handle_scope(isolate); +MaybeLocal MakeCallback(Isolate* isolate, + Local recv, + Local symbol, + int argc, + Local argv[], + async_uid async_id, + async_uid trigger_id) { Local callback_v = recv->Get(symbol); if (callback_v.IsEmpty()) return Local(); if (!callback_v->IsFunction()) return Local(); Local callback = callback_v.As(); - return handle_scope.Escape(MakeCallback(isolate, recv, callback, argc, argv)); + return MakeCallback(isolate, recv, callback, argc, argv, + async_id, trigger_id); } -Local MakeCallback(Isolate* isolate, - Local recv, - Local callback, - int argc, - Local argv[]) { +MaybeLocal MakeCallback(Isolate* isolate, + Local recv, + Local callback, + int argc, + Local argv[], + async_uid async_id, + async_uid trigger_id) { // Observe the following two subtleties: // // 1. The environment is retrieved from the callback function's context. @@ -1434,11 +1417,48 @@ Local MakeCallback(Isolate* isolate, // // Because of the AssignToContext() call in src/node_contextify.cc, // the two contexts need not be the same. - EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(callback->CreationContext()); Context::Scope context_scope(env->context()); + return MakeCallback(env, recv.As(), callback, argc, argv, + async_id, trigger_id); +} + + +// Legacy MakeCallback()s + +Local MakeCallback(Isolate* isolate, + Local recv, + const char* method, + int argc, + Local* argv) { + EscapableHandleScope handle_scope(isolate); + return handle_scope.Escape( + MakeCallback(isolate, recv, method, argc, argv, 0) + .FromMaybe(Local())); +} + + +Local MakeCallback(Isolate* isolate, + Local recv, + Local symbol, + int argc, + Local* argv) { + EscapableHandleScope handle_scope(isolate); + return handle_scope.Escape( + MakeCallback(isolate, recv, symbol, argc, argv, 0) + .FromMaybe(Local())); +} + + +Local MakeCallback(Isolate* isolate, + Local recv, + Local callback, + int argc, + Local* argv) { + EscapableHandleScope handle_scope(isolate); return handle_scope.Escape( - MakeCallback(env, recv.As(), callback, argc, argv)); + MakeCallback(isolate, recv, callback, argc, argv, 0) + .FromMaybe(Local())); } @@ -4382,7 +4402,9 @@ void EmitBeforeExit(Environment* env) { FIXED_ONE_BYTE_STRING(env->isolate(), "beforeExit"), process_object->Get(exit_code)->ToInteger(env->isolate()) }; - MakeCallback(env, process_object, "emit", arraysize(args), args); + MakeCallback(env->isolate(), + process_object, "emit", arraysize(args), args, + 0).ToLocalChecked(); } @@ -4401,7 +4423,9 @@ int EmitExit(Environment* env) { Integer::New(env->isolate(), code) }; - MakeCallback(env, process_object, "emit", arraysize(args), args); + MakeCallback(env->isolate(), + process_object, "emit", arraysize(args), args, + 0).ToLocalChecked(); // Reload exit code, it may be changed by `emit('exit')` return process_object->Get(exitCode)->Int32Value(); diff --git a/src/node.h b/src/node.h index 7ed4521e87dbb2..0627dc112ed00b 100644 --- a/src/node.h +++ b/src/node.h @@ -142,14 +142,10 @@ inline v8::Local UVException(int errorno, } /* - * MakeCallback doesn't have a HandleScope. That means the callers scope - * will retain ownership of created handles from MakeCallback and related. - * There is by default a wrapping HandleScope before uv_run, if the caller - * doesn't have a HandleScope on the stack the global will take ownership - * which won't be reaped until the uv loop exits. + * These methods need to be called in a HandleScope. * - * If a uv callback is fired, and there is no enclosing HandleScope in the - * cb, you will appear to leak 4-bytes for every invocation. Take heed. + * It is preferred that you use the `MakeCallback` overloads taking + * `async_uid` arguments. */ NODE_EXTERN v8::Local MakeCallback( @@ -521,12 +517,123 @@ typedef void (*promise_hook_func) (v8::PromiseHookType type, v8::Local parent, void* arg); +typedef double async_uid; + /* Registers an additional v8::PromiseHook wrapper. This API exists because V8 * itself supports only a single PromiseHook. */ NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg); +/* 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. */ +async_uid AsyncHooksGetCurrentId(v8::Isolate* isolate); + +/* Return same value as async_hooks.triggerId(); */ +async_uid AsyncHooksGetTriggerId(v8::Isolate* isolate); + +/* If the native API doesn't inherit from the helper class then the callbacks + * must be triggered manually. This triggers the init() callback. The return + * value is the uid assigned to the resource. */ +async_uid EmitAsyncInit(v8::Isolate* isolate, + v8::Local resource, + const char* name, + async_uid trigger_id = -1); + +/* Emit the destroy() callback. */ +void EmitAsyncDestroy(v8::Isolate* isolate, async_uid id); + +/* An API specific to emit before/after callbacks is unnecessary because + * MakeCallback will automatically call them for you. + * + * These methods may create handles on their own, so run them inside a + * HandleScope. */ +v8::MaybeLocal MakeCallback(v8::Isolate* isolate, + v8::Local recv, + v8::Local callback, + int argc, + v8::Local* argv, + async_uid asyncId, + async_uid triggerId = -1); +v8::MaybeLocal MakeCallback(v8::Isolate* isolate, + v8::Local recv, + const char* method, + int argc, + v8::Local* argv, + async_uid asyncId, + async_uid triggerId = -1); +v8::MaybeLocal MakeCallback(v8::Isolate* isolate, + v8::Local recv, + v8::Local symbol, + int argc, + v8::Local* argv, + async_uid asyncId, + async_uid triggerId = -1); + +/* Helper class users can optionally inherit from. If + * `AsyncResource::MakeCallback()` is used, then all four callbacks will be + * called automatically. */ +class AsyncResource { + public: + AsyncResource(v8::Isolate* isolate, + v8::Local resource, + const char* name, + async_uid trigger_id = -1) + : isolate_(isolate), + resource_(isolate, resource), + trigger_id_(trigger_id) { + if (trigger_id_ == -1) + trigger_id_ = AsyncHooksGetTriggerId(isolate); + + uid_ = EmitAsyncInit(isolate, resource, name, trigger_id_); + } + + ~AsyncResource() { + EmitAsyncDestroy(isolate_, uid_); + } + + v8::MaybeLocal MakeCallback( + v8::Local callback, + int argc, + v8::Local* argv) { + return node::MakeCallback(isolate_, get_resource(), + callback, argc, argv, + uid_, trigger_id_); + } + + v8::MaybeLocal MakeCallback( + const char* method, + int argc, + v8::Local* argv) { + return node::MakeCallback(isolate_, get_resource(), + method, argc, argv, + uid_, trigger_id_); + } + + v8::MaybeLocal MakeCallback( + v8::Local symbol, + int argc, + v8::Local* argv) { + return node::MakeCallback(isolate_, get_resource(), + symbol, argc, argv, + uid_, trigger_id_); + } + + v8::Local get_resource() { + return resource_.Get(isolate_); + } + + async_uid get_uid() const { + return uid_; + } + private: + v8::Isolate* isolate_; + v8::Persistent resource_; + async_uid uid_; + async_uid trigger_id_; +}; + } // namespace node #endif // SRC_NODE_H_ diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 68613ab5659de0..8fc3acc63520db 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1237,11 +1237,13 @@ int SecureContext::TicketKeyCallback(SSL* ssl, kTicketPartSize).ToLocalChecked(), Boolean::New(env->isolate(), enc != 0) }; - Local ret = node::MakeCallback(env, + + Local ret = node::MakeCallback(env->isolate(), sc->object(), env->ticketkeycallback_string(), arraysize(argv), - argv); + argv, + 0).ToLocalChecked(); Local arr = ret.As(); int r = arr->Get(kTicketKeyReturnIndex)->Int32Value(); diff --git a/src/node_internals.h b/src/node_internals.h index e07cb9d6d39a41..a08ab45affe96e 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -94,27 +94,6 @@ inline v8::Local PersistentToLocal( v8::Isolate* isolate, const v8::Persistent& persistent); -// Call with valid HandleScope and while inside Context scope. -v8::Local MakeCallback(Environment* env, - v8::Local recv, - const char* method, - int argc = 0, - v8::Local* argv = nullptr); - -// Call with valid HandleScope and while inside Context scope. -v8::Local MakeCallback(Environment* env, - v8::Local recv, - v8::Local symbol, - int argc = 0, - v8::Local* argv = nullptr); - -// Call with valid HandleScope and while inside Context scope. -v8::Local MakeCallback(Environment* env, - v8::Local recv, - v8::Local callback, - int argc = 0, - v8::Local* argv = nullptr); - // Convert a struct sockaddr to a { address: '1.2.3.4', port: 1234 } JS object. // Sets address and port properties on the info object and returns it. // If |info| is omitted, a new object is returned. From b2d726b399c1bd80cfad8478a02c294f7a488250 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 25 May 2017 19:56:24 +0200 Subject: [PATCH 2/8] [squash] make MakeCallback assume UTF-8 input for string variant --- src/node.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index 55315bd8f76b60..ffbb666daecbf6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1381,7 +1381,9 @@ MaybeLocal MakeCallback(Isolate* isolate, Local argv[], async_uid async_id, async_uid trigger_id) { - Local method_string = OneByteString(isolate, method); + Local method_string = + String::NewFromUtf8(isolate, method, v8::NewStringType::kNormal) + .ToLocalChecked(); return MakeCallback(isolate, recv, method_string, argc, argv, async_id, trigger_id); } From 82800480828c78797b5edaeb274ee06b515c9caf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 26 May 2017 18:06:54 +0200 Subject: [PATCH 3/8] [squash] add NODE_EXTERN to external APIs --- src/node.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/node.h b/src/node.h index 0627dc112ed00b..dd09a254f04149 100644 --- a/src/node.h +++ b/src/node.h @@ -528,27 +528,28 @@ 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. */ -async_uid AsyncHooksGetCurrentId(v8::Isolate* isolate); +NODE_EXTERN async_uid AsyncHooksGetCurrentId(v8::Isolate* isolate); /* Return same value as async_hooks.triggerId(); */ -async_uid AsyncHooksGetTriggerId(v8::Isolate* isolate); +NODE_EXTERN async_uid AsyncHooksGetTriggerId(v8::Isolate* isolate); /* If the native API doesn't inherit from the helper class then the callbacks * must be triggered manually. This triggers the init() callback. The return * value is the uid assigned to the resource. */ -async_uid EmitAsyncInit(v8::Isolate* isolate, - v8::Local resource, - const char* name, - async_uid trigger_id = -1); +NODE_EXTERN async_uid EmitAsyncInit(v8::Isolate* isolate, + v8::Local resource, + const char* name, + async_uid trigger_id = -1); /* Emit the destroy() callback. */ -void EmitAsyncDestroy(v8::Isolate* isolate, async_uid id); +NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate, async_uid id); /* An API specific to emit before/after callbacks is unnecessary because * MakeCallback will automatically call them for you. * * These methods may create handles on their own, so run them inside a * HandleScope. */ +NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, v8::Local callback, @@ -556,6 +557,7 @@ v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local* argv, async_uid asyncId, async_uid triggerId = -1); +NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, const char* method, @@ -563,6 +565,7 @@ v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local* argv, async_uid asyncId, async_uid triggerId = -1); +NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, v8::Local symbol, From 6881fd6dcc74b7916e64ab2a8ded363e83796940 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 27 May 2017 09:45:07 +0200 Subject: [PATCH 4/8] [squash] make trigger id non-optional --- src/inspector_agent.cc | 2 +- src/node.cc | 14 ++++++-------- src/node.h | 13 +++++++++---- src/node_crypto.cc | 2 +- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 54a8957acf28d7..5daef2e1ba9ffe 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -598,7 +598,7 @@ bool Agent::StartIoThread(bool wait_for_connect) { message }; MakeCallback(parent_env_->isolate(), process_object, emit_fn.As(), - arraysize(argv), argv, 0); + arraysize(argv), argv, 0, 0); return true; } diff --git a/src/node.cc b/src/node.cc index ffbb666daecbf6..8972dd61353518 100644 --- a/src/node.cc +++ b/src/node.cc @@ -353,7 +353,7 @@ static void CheckImmediate(uv_check_t* handle) { env->immediate_callback_string(), 0, nullptr, - 0).ToLocalChecked(); + 0, 0).ToLocalChecked(); } @@ -1314,8 +1314,6 @@ MaybeLocal MakeCallback(Environment* env, MaybeLocal ret; { - if (trigger_id == -1) - trigger_id = async_id; AsyncHooks::ExecScope exec_scope(env, async_id, trigger_id); if (async_id != 0) { @@ -1435,7 +1433,7 @@ Local MakeCallback(Isolate* isolate, Local* argv) { EscapableHandleScope handle_scope(isolate); return handle_scope.Escape( - MakeCallback(isolate, recv, method, argc, argv, 0) + MakeCallback(isolate, recv, method, argc, argv, 0, 0) .FromMaybe(Local())); } @@ -1447,7 +1445,7 @@ Local MakeCallback(Isolate* isolate, Local* argv) { EscapableHandleScope handle_scope(isolate); return handle_scope.Escape( - MakeCallback(isolate, recv, symbol, argc, argv, 0) + MakeCallback(isolate, recv, symbol, argc, argv, 0, 0) .FromMaybe(Local())); } @@ -1459,7 +1457,7 @@ Local MakeCallback(Isolate* isolate, Local* argv) { EscapableHandleScope handle_scope(isolate); return handle_scope.Escape( - MakeCallback(isolate, recv, callback, argc, argv, 0) + MakeCallback(isolate, recv, callback, argc, argv, 0, 0) .FromMaybe(Local())); } @@ -4406,7 +4404,7 @@ void EmitBeforeExit(Environment* env) { }; MakeCallback(env->isolate(), process_object, "emit", arraysize(args), args, - 0).ToLocalChecked(); + 0, 0).ToLocalChecked(); } @@ -4427,7 +4425,7 @@ int EmitExit(Environment* env) { MakeCallback(env->isolate(), process_object, "emit", arraysize(args), args, - 0).ToLocalChecked(); + 0, 0).ToLocalChecked(); // Reload exit code, it may be changed by `emit('exit')` return process_object->Get(exitCode)->Int32Value(); diff --git a/src/node.h b/src/node.h index dd09a254f04149..dcd51a727c2035 100644 --- a/src/node.h +++ b/src/node.h @@ -548,7 +548,12 @@ NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate, async_uid id); * MakeCallback will automatically call them for you. * * These methods may create handles on their own, so run them inside a - * HandleScope. */ + * HandleScope. + * + * `asyncId` and `triggerId` should correspond to the values returned by + * `EmitAsyncInit()` and `AsyncHooksGetTriggerId()`, respectively, when the + * invoking resource was created. If these values are unknown, 0 can be passed. + * */ NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, @@ -556,7 +561,7 @@ v8::MaybeLocal MakeCallback(v8::Isolate* isolate, int argc, v8::Local* argv, async_uid asyncId, - async_uid triggerId = -1); + async_uid triggerId); NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, @@ -564,7 +569,7 @@ v8::MaybeLocal MakeCallback(v8::Isolate* isolate, int argc, v8::Local* argv, async_uid asyncId, - async_uid triggerId = -1); + async_uid triggerId); NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, @@ -572,7 +577,7 @@ v8::MaybeLocal MakeCallback(v8::Isolate* isolate, int argc, v8::Local* argv, async_uid asyncId, - async_uid triggerId = -1); + async_uid triggerId); /* Helper class users can optionally inherit from. If * `AsyncResource::MakeCallback()` is used, then all four callbacks will be diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 8fc3acc63520db..ef9502522946f0 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1243,7 +1243,7 @@ int SecureContext::TicketKeyCallback(SSL* ssl, env->ticketkeycallback_string(), arraysize(argv), argv, - 0).ToLocalChecked(); + 0, 0).ToLocalChecked(); Local arr = ret.As(); int r = arr->Get(kTicketKeyReturnIndex)->Int32Value(); From 6af7d4518b27fc353de5bd9c6594456fc0779405 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 27 May 2017 11:06:09 +0200 Subject: [PATCH 5/8] [squash] make trigger_id non-optional in EmitAsyncInit --- src/async-wrap.cc | 2 -- src/node.h | 8 ++++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 5a8afe390f7833..2efe0e23ecb72f 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -665,8 +665,6 @@ async_uid EmitAsyncInit(Isolate* isolate, async_uid trigger_id) { Environment* env = Environment::GetCurrent(isolate); async_uid async_id = env->new_async_id(); - if (trigger_id == -1) - trigger_id = env->get_init_trigger_id(); Local type = String::NewFromUtf8(isolate, name, v8::NewStringType::kInternalized) diff --git a/src/node.h b/src/node.h index dcd51a727c2035..329a3c623bddab 100644 --- a/src/node.h +++ b/src/node.h @@ -535,11 +535,15 @@ NODE_EXTERN async_uid AsyncHooksGetTriggerId(v8::Isolate* isolate); /* If the native API doesn't inherit from the helper class then the callbacks * must be triggered manually. This triggers the init() callback. The return - * value is the uid assigned to the resource. */ + * value is the uid assigned to the resource. + * + * The `trigger_id` parameter should correspond to the resource which is + * creating the new resource, which will usually be the return value of + * `AsyncHooksGetTriggerId()`. */ NODE_EXTERN async_uid EmitAsyncInit(v8::Isolate* isolate, v8::Local resource, const char* name, - async_uid trigger_id = -1); + async_uid trigger_id); /* Emit the destroy() callback. */ NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate, async_uid id); From 7c0aaf737e22bb80e2aa0248733af49304cf07d3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 25 May 2017 19:56:50 +0200 Subject: [PATCH 6/8] test: add AsyncResource addon test --- test/addons/async-resource/binding.cc | 86 ++++++++++++++++++++++++++ test/addons/async-resource/binding.gyp | 9 +++ test/addons/async-resource/test.js | 53 ++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 test/addons/async-resource/binding.cc create mode 100644 test/addons/async-resource/binding.gyp create mode 100644 test/addons/async-resource/test.js diff --git a/test/addons/async-resource/binding.cc b/test/addons/async-resource/binding.cc new file mode 100644 index 00000000000000..2321ef1d217719 --- /dev/null +++ b/test/addons/async-resource/binding.cc @@ -0,0 +1,86 @@ +#include "node.h" + +#include +#include + +namespace { + +using node::AsyncResource; +using v8::External; +using v8::Function; +using v8::FunctionCallbackInfo; +using v8::Integer; +using v8::Isolate; +using v8::Local; +using v8::MaybeLocal; +using v8::Object; +using v8::String; +using v8::Value; + +void CreateAsyncResource(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + assert(args[0]->IsObject()); + AsyncResource* r = new AsyncResource(isolate, args[0].As(), "foobär"); + args.GetReturnValue().Set( + External::New(isolate, static_cast(r))); +} + +void DestroyAsyncResource(const FunctionCallbackInfo& args) { + assert(args[0]->IsExternal()); + auto r = static_cast(args[0].As()->Value()); + delete r; +} + +void CallViaFunction(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + assert(args[0]->IsExternal()); + auto r = static_cast(args[0].As()->Value()); + + Local name = + String::NewFromUtf8(isolate, "methöd", v8::NewStringType::kNormal) + .ToLocalChecked(); + Local fn = + r->get_resource()->Get(isolate->GetCurrentContext(), name) + .ToLocalChecked(); + assert(fn->IsFunction()); + + Local arg = Integer::New(isolate, 42); + MaybeLocal ret = r->MakeCallback(fn.As(), 1, &arg); + args.GetReturnValue().Set(ret.FromMaybe(Local())); +} + +void CallViaString(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + assert(args[0]->IsExternal()); + auto r = static_cast(args[0].As()->Value()); + + Local name = + String::NewFromUtf8(isolate, "methöd", v8::NewStringType::kNormal) + .ToLocalChecked(); + + Local arg = Integer::New(isolate, 42); + MaybeLocal ret = r->MakeCallback(name, 1, &arg); + args.GetReturnValue().Set(ret.FromMaybe(Local())); +} + +void CallViaUtf8Name(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + assert(args[0]->IsExternal()); + auto r = static_cast(args[0].As()->Value()); + + Local arg = Integer::New(isolate, 42); + MaybeLocal ret = r->MakeCallback("methöd", 1, &arg); + args.GetReturnValue().Set(ret.FromMaybe(Local())); +} + +void Initialize(Local exports) { + NODE_SET_METHOD(exports, "createAsyncResource", CreateAsyncResource); + NODE_SET_METHOD(exports, "destroyAsyncResource", DestroyAsyncResource); + NODE_SET_METHOD(exports, "callViaFunction", CallViaFunction); + NODE_SET_METHOD(exports, "callViaString", CallViaString); + NODE_SET_METHOD(exports, "callViaUtf8Name", CallViaUtf8Name); +} + +} // namespace + +NODE_MODULE(binding, Initialize) diff --git a/test/addons/async-resource/binding.gyp b/test/addons/async-resource/binding.gyp new file mode 100644 index 00000000000000..7ede63d94a0d77 --- /dev/null +++ b/test/addons/async-resource/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons/async-resource/test.js b/test/addons/async-resource/test.js new file mode 100644 index 00000000000000..044ceeb85b49a5 --- /dev/null +++ b/test/addons/async-resource/test.js @@ -0,0 +1,53 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const binding = require(`./build/${common.buildType}/binding`); +const async_hooks = require('async_hooks'); + +const bindingUids = []; +let before = 0; +let after = 0; +let destroy = 0; + +async_hooks.createHook({ + init(id, type, triggerId, resource) { + if (type === 'foobär') bindingUids.push(id); + }, + + before(id) { + if (bindingUids.includes(id)) before++; + }, + + after(id) { + if (bindingUids.includes(id)) after++; + }, + + destroy(id) { + if (bindingUids.includes(id)) destroy++; + } +}).enable(); + +for (const call of [binding.callViaFunction, + binding.callViaString, + binding.callViaUtf8Name]) { + const object = { + methöd(arg) { + assert.strictEqual(this, object); + assert.strictEqual(arg, 42); + return 'baz'; + } + }; + + const resource = binding.createAsyncResource(object); + const ret = call(resource); + assert.strictEqual(ret, 'baz'); + binding.destroyAsyncResource(resource); +} + +setImmediate(common.mustCall(() => { + assert.strictEqual(bindingUids.length, 3); + assert.strictEqual(before, 3); + assert.strictEqual(after, 3); + assert.strictEqual(destroy, 3); +})); From 9d220a2c948806250bf616613980bc41888a4433 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 25 May 2017 21:57:56 +0200 Subject: [PATCH 7/8] [squash] more tests --- test/addons/async-resource/binding.cc | 23 ++++++++++++- test/addons/async-resource/test.js | 48 ++++++++++++++++++--------- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/test/addons/async-resource/binding.cc b/test/addons/async-resource/binding.cc index 2321ef1d217719..93e3d791d7471e 100644 --- a/test/addons/async-resource/binding.cc +++ b/test/addons/async-resource/binding.cc @@ -20,7 +20,14 @@ using v8::Value; void CreateAsyncResource(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); assert(args[0]->IsObject()); - AsyncResource* r = new AsyncResource(isolate, args[0].As(), "foobär"); + AsyncResource* r; + if (args[1]->IsInt32()) { + r = new AsyncResource(isolate, args[0].As(), "foobär", + args[1].As()->Value()); + } else { + r = new AsyncResource(isolate, args[0].As(), "foobär"); + } + args.GetReturnValue().Set( External::New(isolate, static_cast(r))); } @@ -73,12 +80,26 @@ void CallViaUtf8Name(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret.FromMaybe(Local())); } +void GetUid(const FunctionCallbackInfo& args) { + assert(args[0]->IsExternal()); + auto r = static_cast(args[0].As()->Value()); + args.GetReturnValue().Set(r->get_uid()); +} + +void GetResource(const FunctionCallbackInfo& args) { + assert(args[0]->IsExternal()); + auto r = static_cast(args[0].As()->Value()); + args.GetReturnValue().Set(r->get_resource()); +} + void Initialize(Local exports) { NODE_SET_METHOD(exports, "createAsyncResource", CreateAsyncResource); NODE_SET_METHOD(exports, "destroyAsyncResource", DestroyAsyncResource); NODE_SET_METHOD(exports, "callViaFunction", CallViaFunction); NODE_SET_METHOD(exports, "callViaString", CallViaString); NODE_SET_METHOD(exports, "callViaUtf8Name", CallViaUtf8Name); + NODE_SET_METHOD(exports, "getUid", GetUid); + NODE_SET_METHOD(exports, "getResource", GetResource); } } // namespace diff --git a/test/addons/async-resource/test.js b/test/addons/async-resource/test.js index 044ceeb85b49a5..e2a11bdf1addb8 100644 --- a/test/addons/async-resource/test.js +++ b/test/addons/async-resource/test.js @@ -6,13 +6,17 @@ const binding = require(`./build/${common.buildType}/binding`); const async_hooks = require('async_hooks'); const bindingUids = []; +let expectedTriggerId; let before = 0; let after = 0; let destroy = 0; async_hooks.createHook({ init(id, type, triggerId, resource) { - if (type === 'foobär') bindingUids.push(id); + if (type === 'foobär') { + assert.strictEqual(triggerId, expectedTriggerId); + bindingUids.push(id); + } }, before(id) { @@ -31,23 +35,35 @@ async_hooks.createHook({ for (const call of [binding.callViaFunction, binding.callViaString, binding.callViaUtf8Name]) { - const object = { - methöd(arg) { - assert.strictEqual(this, object); - assert.strictEqual(arg, 42); - return 'baz'; - } - }; + for (const passedTriggerId of [undefined, 12345]) { + const object = { + methöd(arg) { + assert.strictEqual(this, object); + assert.strictEqual(arg, 42); + return 'baz'; + } + }; + + if (passedTriggerId === undefined) + expectedTriggerId = 1; + else + expectedTriggerId = passedTriggerId; - const resource = binding.createAsyncResource(object); - const ret = call(resource); - assert.strictEqual(ret, 'baz'); - binding.destroyAsyncResource(resource); + const resource = binding.createAsyncResource(object, passedTriggerId); + const ret = call(resource); + assert.strictEqual(ret, 'baz'); + assert.strictEqual(binding.getResource(resource), object); + + const mostRecentUid = bindingUids[bindingUids.length - 1]; + assert.strictEqual(binding.getUid(resource), mostRecentUid); + + binding.destroyAsyncResource(resource); + } } setImmediate(common.mustCall(() => { - assert.strictEqual(bindingUids.length, 3); - assert.strictEqual(before, 3); - assert.strictEqual(after, 3); - assert.strictEqual(destroy, 3); + assert.strictEqual(bindingUids.length, 6); + assert.strictEqual(before, bindingUids.length); + assert.strictEqual(after, bindingUids.length); + assert.strictEqual(destroy, bindingUids.length); })); From aa643fdac14524864da167dcd3ba2f3241fd3e96 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 25 May 2017 22:10:59 +0200 Subject: [PATCH 8/8] [squash] tighten up test --- test/addons/async-resource/binding.cc | 5 +++++ test/addons/async-resource/test.js | 19 +++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/test/addons/async-resource/binding.cc b/test/addons/async-resource/binding.cc index 93e3d791d7471e..093adfcba6aca0 100644 --- a/test/addons/async-resource/binding.cc +++ b/test/addons/async-resource/binding.cc @@ -92,6 +92,10 @@ void GetResource(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(r->get_resource()); } +void GetCurrentId(const FunctionCallbackInfo& args) { + args.GetReturnValue().Set(node::AsyncHooksGetCurrentId(args.GetIsolate())); +} + void Initialize(Local exports) { NODE_SET_METHOD(exports, "createAsyncResource", CreateAsyncResource); NODE_SET_METHOD(exports, "destroyAsyncResource", DestroyAsyncResource); @@ -100,6 +104,7 @@ void Initialize(Local exports) { NODE_SET_METHOD(exports, "callViaUtf8Name", CallViaUtf8Name); NODE_SET_METHOD(exports, "getUid", GetUid); NODE_SET_METHOD(exports, "getResource", GetResource); + NODE_SET_METHOD(exports, "getCurrentId", GetCurrentId); } } // namespace diff --git a/test/addons/async-resource/test.js b/test/addons/async-resource/test.js index e2a11bdf1addb8..34017750ce500e 100644 --- a/test/addons/async-resource/test.js +++ b/test/addons/async-resource/test.js @@ -5,6 +5,8 @@ const assert = require('assert'); const binding = require(`./build/${common.buildType}/binding`); const async_hooks = require('async_hooks'); +const kObjectTag = Symbol('kObjectTag'); + const bindingUids = []; let expectedTriggerId; let before = 0; @@ -13,7 +15,11 @@ let destroy = 0; async_hooks.createHook({ init(id, type, triggerId, resource) { + assert.strictEqual(typeof id, 'number'); + assert.strictEqual(typeof resource, 'object'); + assert(id > 1); if (type === 'foobär') { + assert.strictEqual(resource.kObjectTag, kObjectTag); assert.strictEqual(triggerId, expectedTriggerId); bindingUids.push(id); } @@ -32,16 +38,21 @@ async_hooks.createHook({ } }).enable(); +assert.strictEqual(binding.getCurrentId(), 1); + for (const call of [binding.callViaFunction, binding.callViaString, binding.callViaUtf8Name]) { for (const passedTriggerId of [undefined, 12345]) { + let uid; const object = { methöd(arg) { assert.strictEqual(this, object); assert.strictEqual(arg, 42); + assert.strictEqual(binding.getCurrentId(), uid); return 'baz'; - } + }, + kObjectTag }; if (passedTriggerId === undefined) @@ -50,12 +61,12 @@ for (const call of [binding.callViaFunction, expectedTriggerId = passedTriggerId; const resource = binding.createAsyncResource(object, passedTriggerId); + uid = bindingUids[bindingUids.length - 1]; + const ret = call(resource); assert.strictEqual(ret, 'baz'); assert.strictEqual(binding.getResource(resource), object); - - const mostRecentUid = bindingUids[bindingUids.length - 1]; - assert.strictEqual(binding.getUid(resource), mostRecentUid); + assert.strictEqual(binding.getUid(resource), uid); binding.destroyAsyncResource(resource); }