From 3685f0447da42afdf42550f6d2171b7283f9d794 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Thu, 6 Jul 2017 08:20:03 +0200 Subject: [PATCH] async_hooks: C++ Embedder API overhaul * 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: https://github.com/nodejs/node/pull/14040 Reviewed-By: Trevor Norris Reviewed-By: Anna Henningsen --- src/async-wrap.cc | 39 +++++++++----- src/inspector_agent.cc | 2 +- src/node.cc | 49 ++++++++---------- src/node.h | 72 ++++++++++++++------------ src/node_crypto.cc | 2 +- test/addons/async-hooks-id/binding.cc | 27 ++++++++++ test/addons/async-hooks-id/binding.gyp | 9 ++++ test/addons/async-hooks-id/test.js | 26 ++++++++++ test/addons/async-resource/binding.cc | 19 +++---- test/addons/async-resource/test.js | 10 ++-- 10 files changed, 167 insertions(+), 88 deletions(-) create mode 100644 test/addons/async-hooks-id/binding.cc create mode 100644 test/addons/async-hooks-id/binding.gyp create mode 100644 test/addons/async-hooks-id/test.js diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 7cfa9e0fe817aa..0afaf663716ad2 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -741,40 +741,51 @@ Local AsyncWrap::MakeCallback(const Local 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 resource, - const char* name, - async_uid trigger_id) { +async_context EmitAsyncInit(Isolate* isolate, + Local 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 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 diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index bfa2b082b4ef35..a06ff032ff7d51 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -604,7 +604,7 @@ bool Agent::StartIoThread(bool wait_for_connect) { message }; MakeCallback(parent_env_->isolate(), process_object, emit_fn.As(), - arraysize(argv), argv, 0, 0); + arraysize(argv), argv, {0, 0}); return true; } diff --git a/src/node.cc b/src/node.cc index 305f61945b4a27..741b5dea82b7ba 100644 --- a/src/node.cc +++ b/src/node.cc @@ -363,7 +363,7 @@ static void CheckImmediate(uv_check_t* handle) { env->immediate_callback_string(), 0, nullptr, - 0, 0).ToLocalChecked(); + {0, 0}).ToLocalChecked(); } @@ -1298,8 +1298,7 @@ MaybeLocal MakeCallback(Environment* env, const Local callback, int argc, Local 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()); @@ -1321,10 +1320,12 @@ MaybeLocal MakeCallback(Environment* env, MaybeLocal 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(); + if (asyncContext.async_id != 0) { + if (!AsyncWrap::EmitBefore(env, asyncContext.async_id)) + return Local(); } ret = callback->Call(env->context(), recv, argc, argv); @@ -1336,8 +1337,9 @@ MaybeLocal MakeCallback(Environment* env, ret : Undefined(env->isolate()); } - if (async_id != 0) { - if (!AsyncWrap::EmitAfter(env, async_id)) return Local(); + if (asyncContext.async_id != 0) { + if (!AsyncWrap::EmitAfter(env, asyncContext.async_id)) + return Local(); } } @@ -1358,8 +1360,8 @@ MaybeLocal 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 process = env->process_object(); @@ -1384,13 +1386,11 @@ MaybeLocal MakeCallback(Isolate* isolate, const char* method, int argc, Local argv[], - async_uid async_id, - async_uid trigger_id) { + async_context asyncContext) { Local 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); } @@ -1399,14 +1399,12 @@ MaybeLocal MakeCallback(Isolate* isolate, Local symbol, int argc, Local argv[], - async_uid async_id, - async_uid trigger_id) { + async_context asyncContext) { Local callback_v = recv->Get(symbol); if (callback_v.IsEmpty()) return Local(); if (!callback_v->IsFunction()) return Local(); Local callback = callback_v.As(); - return MakeCallback(isolate, recv, callback, argc, argv, - async_id, trigger_id); + return MakeCallback(isolate, recv, callback, argc, argv, asyncContext); } @@ -1415,8 +1413,7 @@ MaybeLocal MakeCallback(Isolate* isolate, Local callback, int argc, Local 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. @@ -1427,7 +1424,7 @@ MaybeLocal MakeCallback(Isolate* 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); + asyncContext); } @@ -1440,7 +1437,7 @@ Local MakeCallback(Isolate* isolate, Local* 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())); } @@ -1452,7 +1449,7 @@ Local MakeCallback(Isolate* isolate, Local* 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())); } @@ -1464,7 +1461,7 @@ Local MakeCallback(Isolate* isolate, Local* 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())); } @@ -4448,7 +4445,7 @@ void EmitBeforeExit(Environment* env) { }; MakeCallback(env->isolate(), process_object, "emit", arraysize(args), args, - 0, 0).ToLocalChecked(); + {0, 0}).ToLocalChecked(); } @@ -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(); diff --git a/src/node.h b/src/node.h index 9e4edd06277b1e..88465a763476b7 100644 --- a/src/node.h +++ b/src/node.h @@ -145,7 +145,7 @@ inline v8::Local 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 MakeCallback( @@ -517,7 +517,11 @@ typedef void (*promise_hook_func) (v8::PromiseHookType type, v8::Local 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. */ @@ -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 @@ -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 resource, - const char* name, - async_uid trigger_async_id); +NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate, + v8::Local 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. @@ -572,24 +577,21 @@ v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local callback, int argc, v8::Local* argv, - async_uid asyncId, - async_uid triggerAsyncId); + async_context asyncContext); NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, const char* method, int argc, v8::Local* argv, - async_uid asyncId, - async_uid triggerAsyncId); + async_context asyncContext); NODE_EXTERN v8::MaybeLocal MakeCallback(v8::Isolate* isolate, v8::Local recv, v8::Local symbol, int argc, v8::Local* 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 @@ -599,18 +601,15 @@ class AsyncResource { AsyncResource(v8::Isolate* isolate, v8::Local 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 MakeCallback( @@ -619,7 +618,7 @@ class AsyncResource { v8::Local* argv) { return node::MakeCallback(isolate_, get_resource(), callback, argc, argv, - uid_, trigger_async_id_); + async_context_); } v8::MaybeLocal MakeCallback( @@ -628,7 +627,7 @@ class AsyncResource { v8::Local* argv) { return node::MakeCallback(isolate_, get_resource(), method, argc, argv, - uid_, trigger_async_id_); + async_context_); } v8::MaybeLocal MakeCallback( @@ -637,21 +636,30 @@ class AsyncResource { v8::Local* argv) { return node::MakeCallback(isolate_, get_resource(), symbol, argc, argv, - uid_, trigger_async_id_); + async_context_); } v8::Local 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 resource_; - async_uid uid_; - async_uid trigger_async_id_; + async_context async_context_; }; } // namespace node diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 9d196c10cc8c3f..36102e53f9c045 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1237,7 +1237,7 @@ int SecureContext::TicketKeyCallback(SSL* ssl, env->ticketkeycallback_string(), arraysize(argv), argv, - 0, 0).ToLocalChecked(); + {0, 0}).ToLocalChecked(); Local arr = ret.As(); int r = arr->Get(kTicketKeyReturnIndex)->Int32Value(); diff --git a/test/addons/async-hooks-id/binding.cc b/test/addons/async-hooks-id/binding.cc new file mode 100644 index 00000000000000..611c250e9a27e4 --- /dev/null +++ b/test/addons/async-hooks-id/binding.cc @@ -0,0 +1,27 @@ +#include "node.h" + +namespace { + +using v8::FunctionCallbackInfo; +using v8::Local; +using v8::Object; +using v8::Value; + +void GetExecutionAsyncId(const FunctionCallbackInfo& args) { + args.GetReturnValue().Set( + node::AsyncHooksGetExecutionAsyncId(args.GetIsolate())); +} + +void GetTriggerAsyncId(const FunctionCallbackInfo& args) { + args.GetReturnValue().Set( + node::AsyncHooksGetTriggerAsyncId(args.GetIsolate())); +} + +void Initialize(Local exports) { + NODE_SET_METHOD(exports, "getExecutionAsyncId", GetExecutionAsyncId); + NODE_SET_METHOD(exports, "getTriggerAsyncId", GetTriggerAsyncId); +} + +} // namespace + +NODE_MODULE(binding, Initialize) diff --git a/test/addons/async-hooks-id/binding.gyp b/test/addons/async-hooks-id/binding.gyp new file mode 100644 index 00000000000000..7ede63d94a0d77 --- /dev/null +++ b/test/addons/async-hooks-id/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons/async-hooks-id/test.js b/test/addons/async-hooks-id/test.js new file mode 100644 index 00000000000000..e6c3cf612cacd5 --- /dev/null +++ b/test/addons/async-hooks-id/test.js @@ -0,0 +1,26 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const binding = require(`./build/${common.buildType}/binding`); +const async_hooks = require('async_hooks'); + +assert.strictEqual( + binding.getExecutionAsyncId(), + async_hooks.executionAsyncId() +); +assert.strictEqual( + binding.getTriggerAsyncId(), + async_hooks.triggerAsyncId() +); + +process.nextTick(common.mustCall(function() { + assert.strictEqual( + binding.getExecutionAsyncId(), + async_hooks.executionAsyncId() + ); + assert.strictEqual( + binding.getTriggerAsyncId(), + async_hooks.triggerAsyncId() + ); +})); diff --git a/test/addons/async-resource/binding.cc b/test/addons/async-resource/binding.cc index 372f7a6fa464ed..9d3ab37e12a838 100644 --- a/test/addons/async-resource/binding.cc +++ b/test/addons/async-resource/binding.cc @@ -80,21 +80,22 @@ void CallViaUtf8Name(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret.FromMaybe(Local())); } -void GetUid(const FunctionCallbackInfo& args) { +void GetAsyncId(const FunctionCallbackInfo& args) { assert(args[0]->IsExternal()); auto r = static_cast(args[0].As()->Value()); - args.GetReturnValue().Set(r->get_uid()); + args.GetReturnValue().Set(r->get_async_id()); } -void GetResource(const FunctionCallbackInfo& args) { +void GetTriggerAsyncId(const FunctionCallbackInfo& args) { assert(args[0]->IsExternal()); auto r = static_cast(args[0].As()->Value()); - args.GetReturnValue().Set(r->get_resource()); + args.GetReturnValue().Set(r->get_trigger_async_id()); } -void GetCurrentId(const FunctionCallbackInfo& args) { - args.GetReturnValue().Set( - node::AsyncHooksGetExecutionAsyncId(args.GetIsolate())); +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) { @@ -103,9 +104,9 @@ void Initialize(Local exports) { 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, "getAsyncId", GetAsyncId); + NODE_SET_METHOD(exports, "getTriggerAsyncId", GetTriggerAsyncId); 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 b52db61a95c1ba..f19ad58637f187 100644 --- a/test/addons/async-resource/test.js +++ b/test/addons/async-resource/test.js @@ -6,6 +6,7 @@ const binding = require(`./build/${common.buildType}/binding`); const async_hooks = require('async_hooks'); const kObjectTag = Symbol('kObjectTag'); +const rootAsyncId = async_hooks.executionAsyncId(); const bindingUids = []; let expectedTriggerId; @@ -38,8 +39,6 @@ async_hooks.createHook({ } }).enable(); -assert.strictEqual(binding.getCurrentId(), 1); - for (const call of [binding.callViaFunction, binding.callViaString, binding.callViaUtf8Name]) { @@ -49,14 +48,14 @@ for (const call of [binding.callViaFunction, methöd(arg) { assert.strictEqual(this, object); assert.strictEqual(arg, 42); - assert.strictEqual(binding.getCurrentId(), uid); + assert.strictEqual(async_hooks.executionAsyncId(), uid); return 'baz'; }, kObjectTag }; if (passedTriggerId === undefined) - expectedTriggerId = 1; + expectedTriggerId = rootAsyncId; else expectedTriggerId = passedTriggerId; @@ -66,7 +65,8 @@ for (const call of [binding.callViaFunction, const ret = call(resource); assert.strictEqual(ret, 'baz'); assert.strictEqual(binding.getResource(resource), object); - assert.strictEqual(binding.getUid(resource), uid); + assert.strictEqual(binding.getAsyncId(resource), uid); + assert.strictEqual(binding.getTriggerAsyncId(resource), expectedTriggerId); binding.destroyAsyncResource(resource); }