From 13296d1abf78364aa80bf693b0ca8b33de60d0e2 Mon Sep 17 00:00:00 2001 From: Stephen Belanger Date: Wed, 23 Jun 2021 22:35:46 -0700 Subject: [PATCH] async_hooks: eliminate native PromiseHook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/39135 Backport-PR-URL: https://github.com/nodejs/node/pull/39742 Reviewed-By: Rich Trott Reviewed-By: Benjamin Gruenbaum Reviewed-By: Gerhard Stöbich --- lib/internal/async_hooks.js | 49 ++-- src/async_wrap.cc | 284 -------------------- test/addons/async-hooks-promise/binding.cc | 41 --- test/addons/async-hooks-promise/binding.gyp | 9 - test/addons/async-hooks-promise/test.js | 104 ------- 5 files changed, 34 insertions(+), 453 deletions(-) delete mode 100644 test/addons/async-hooks-promise/binding.cc delete mode 100644 test/addons/async-hooks-promise/binding.gyp delete mode 100644 test/addons/async-hooks-promise/test.js diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index fbbf0225606728..42f4dceb77413a 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -52,7 +52,7 @@ const { clearAsyncIdStack, } = async_wrap; // For performance reasons, only track Promises when a hook is enabled. -const { enablePromiseHook, disablePromiseHook, setPromiseHooks } = async_wrap; +const { setPromiseHooks } = async_wrap; // Properties in active_hooks are used to keep track of the set of hooks being // executed in case another hook is enabled/disabled. The new set of hooks is // then restored once the active set of hooks is finished executing. @@ -307,9 +307,13 @@ function trackPromise(promise, parent) { return; } - promise[async_id_symbol] = newAsyncId(); - promise[trigger_async_id_symbol] = parent ? getOrSetAsyncId(parent) : + // Get trigger id from parent async id before making the async id of the + // child so if a new one must be made it will be lower than the child. + const triggerAsyncId = parent ? getOrSetAsyncId(parent) : getDefaultTriggerAsyncId(); + + promise[async_id_symbol] = newAsyncId(); + promise[trigger_async_id_symbol] = triggerAsyncId; } function promiseInitHook(promise, parent) { @@ -319,6 +323,21 @@ function promiseInitHook(promise, parent) { emitInitScript(asyncId, 'PROMISE', triggerAsyncId, promise); } +function promiseInitHookWithDestroyTracking(promise, parent) { + promiseInitHook(promise, parent); + destroyTracking(promise, parent); +} + +const destroyedSymbol = Symbol('destroyed'); + +function destroyTracking(promise, parent) { + trackPromise(promise, parent); + const asyncId = promise[async_id_symbol]; + const destroyed = { destroyed: false }; + promise[destroyedSymbol] = destroyed; + registerDestroyHook(promise, asyncId, destroyed); +} + function promiseBeforeHook(promise) { trackPromise(promise); const asyncId = promise[async_id_symbol]; @@ -355,18 +374,19 @@ function enableHooks() { function updatePromiseHookMode() { wantPromiseHook = true; - if (destroyHooksExist()) { - enablePromiseHook(); - setPromiseHooks(undefined, undefined, undefined, undefined); - } else { - disablePromiseHook(); - setPromiseHooks( - initHooksExist() ? promiseInitHook : undefined, - promiseBeforeHook, - promiseAfterHook, - promiseResolveHooksExist() ? promiseResolveHook : undefined, - ); + let initHook; + if (initHooksExist()) { + initHook = destroyHooksExist() ? promiseInitHookWithDestroyTracking : + promiseInitHook; + } else if (destroyHooksExist()) { + initHook = destroyTracking; } + setPromiseHooks( + initHook, + promiseBeforeHook, + promiseAfterHook, + promiseResolveHooksExist() ? promiseResolveHook : undefined, + ); } function disableHooks() { @@ -381,7 +401,6 @@ function disableHooks() { function disablePromiseHookIfNecessary() { if (!wantPromiseHook) { - disablePromiseHook(); setPromiseHooks(undefined, undefined, undefined, undefined); } } diff --git a/src/async_wrap.cc b/src/async_wrap.cc index c26fcd43bc1ac6..7590ab0197dfd6 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -38,19 +38,12 @@ using v8::Global; using v8::HandleScope; using v8::Integer; using v8::Isolate; -using v8::Just; using v8::Local; -using v8::Maybe; using v8::MaybeLocal; -using v8::Name; using v8::Nothing; using v8::Number; using v8::Object; -using v8::ObjectTemplate; -using v8::Promise; -using v8::PromiseHookType; using v8::PropertyAttribute; -using v8::PropertyCallbackInfo; using v8::ReadOnly; using v8::String; using v8::Uint32; @@ -200,255 +193,6 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) { // TODO(addaleax): Remove once we're on C++17. constexpr double AsyncWrap::kInvalidAsyncId; -static Maybe GetAssignedPromiseAsyncId(Environment* env, - Local promise, - Local id_symbol) { - Local maybe_async_id; - if (!promise->Get(env->context(), id_symbol).ToLocal(&maybe_async_id)) { - return Nothing(); - } - return maybe_async_id->IsNumber() - ? maybe_async_id->NumberValue(env->context()) - : Just(AsyncWrap::kInvalidAsyncId); -} - -class PromiseWrap : public AsyncWrap { - public: - PromiseWrap(Environment* env, Local object, bool silent) - : AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) { - MakeWeak(); - } - - PromiseWrap(Environment* env, Local object, double asyncId, - double triggerAsyncId) - : AsyncWrap(env, object, PROVIDER_PROMISE, asyncId, triggerAsyncId) { - MakeWeak(); - } - - SET_NO_MEMORY_INFO() - SET_MEMORY_INFO_NAME(PromiseWrap) - SET_SELF_SIZE(PromiseWrap) - - static PromiseWrap* New(Environment* env, - Local promise, - bool silent); - static void GetAsyncId(Local property, - const PropertyCallbackInfo& args); - static void GetTriggerAsyncId(Local property, - const PropertyCallbackInfo& args); - - static void Initialize(Environment* env); -}; - -PromiseWrap* PromiseWrap::New(Environment* env, - Local promise, - bool silent) { - Local context = env->context(); - - Local obj; - if (!env->promise_wrap_template()->NewInstance(context).ToLocal(&obj)) - return nullptr; - - CHECK_NULL(promise->GetAlignedPointerFromInternalField(0)); - promise->SetInternalField(0, obj); - - // Skip for init events - if (silent) { - double async_id; - double trigger_async_id; - if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol()) - .To(&async_id)) return nullptr; - if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol()) - .To(&trigger_async_id)) return nullptr; - - if (async_id != AsyncWrap::kInvalidAsyncId && - trigger_async_id != AsyncWrap::kInvalidAsyncId) { - return new PromiseWrap( - env, obj, async_id, trigger_async_id); - } - } - - return new PromiseWrap(env, obj, silent); -} - -void PromiseWrap::GetAsyncId(Local property, - const PropertyCallbackInfo& info) { - Isolate* isolate = info.GetIsolate(); - HandleScope scope(isolate); - - PromiseWrap* wrap = Unwrap(info.Holder()); - double value = wrap->get_async_id(); - - info.GetReturnValue().Set(Number::New(isolate, value)); -} - -void PromiseWrap::GetTriggerAsyncId(Local property, - const PropertyCallbackInfo& info) { - Isolate* isolate = info.GetIsolate(); - HandleScope scope(isolate); - - PromiseWrap* wrap = Unwrap(info.Holder()); - double value = wrap->get_trigger_async_id(); - - info.GetReturnValue().Set(Number::New(isolate, value)); -} - -void PromiseWrap::Initialize(Environment* env) { - Isolate* isolate = env->isolate(); - HandleScope scope(isolate); - - Local ctor = FunctionTemplate::New(isolate); - ctor->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "PromiseWrap")); - - Local promise_wrap_template = ctor->InstanceTemplate(); - env->set_promise_wrap_template(promise_wrap_template); - - promise_wrap_template->SetInternalFieldCount( - PromiseWrap::kInternalFieldCount); - - promise_wrap_template->SetAccessor( - env->async_id_symbol(), - PromiseWrap::GetAsyncId); - - promise_wrap_template->SetAccessor( - env->trigger_async_id_symbol(), - PromiseWrap::GetTriggerAsyncId); -} - -static PromiseWrap* extractPromiseWrap(Local promise) { - // This check is imperfect. If the internal field is set, it should - // be an object. If it's not, we just ignore it. Ideally v8 would - // have had GetInternalField returning a MaybeLocal but this works - // for now. - Local obj = promise->GetInternalField(0); - return obj->IsObject() ? Unwrap(obj.As()) : nullptr; -} - -static uint16_t ToAsyncHooksType(PromiseHookType type) { - switch (type) { - case PromiseHookType::kInit: return AsyncHooks::kInit; - case PromiseHookType::kBefore: return AsyncHooks::kBefore; - case PromiseHookType::kAfter: return AsyncHooks::kAfter; - case PromiseHookType::kResolve: return AsyncHooks::kPromiseResolve; - } - UNREACHABLE(); -} - -// Simplified JavaScript hook fast-path for when there is no destroy hook -static void FastPromiseHook(PromiseHookType type, Local promise, - Local parent) { - Local context = promise->CreationContext(); - Environment* env = Environment::GetCurrent(context); - if (env == nullptr) return; - - if (type == PromiseHookType::kBefore && - env->async_hooks()->fields()[AsyncHooks::kBefore] == 0) { - double async_id; - double trigger_async_id; - if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol()) - .To(&async_id)) return; - if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol()) - .To(&trigger_async_id)) return; - - if (async_id != AsyncWrap::kInvalidAsyncId && - trigger_async_id != AsyncWrap::kInvalidAsyncId) { - env->async_hooks()->push_async_context( - async_id, trigger_async_id, promise); - return; - } - } - - if (type == PromiseHookType::kAfter && - env->async_hooks()->fields()[AsyncHooks::kAfter] == 0) { - double async_id; - if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol()) - .To(&async_id)) return; - - if (async_id != AsyncWrap::kInvalidAsyncId) { - if (env->execution_async_id() == async_id) { - // This condition might not be true if async_hooks was enabled during - // the promise callback execution. - env->async_hooks()->pop_async_context(async_id); - } - return; - } - } - - if (type == PromiseHookType::kResolve && - env->async_hooks()->fields()[AsyncHooks::kPromiseResolve] == 0) { - return; - } - - // Getting up to this point means either init type or - // that there are active hooks of another type. - // In both cases fast-path JS hook should be called. - - Local argv[] = { - Integer::New(env->isolate(), ToAsyncHooksType(type)), - promise, - parent - }; - - TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); - Local promise_hook = env->promise_hook_handler(); - USE(promise_hook->Call(context, Undefined(env->isolate()), 3, argv)); -} - -static void FullPromiseHook(PromiseHookType type, Local promise, - Local parent) { - Local context = promise->CreationContext(); - - Environment* env = Environment::GetCurrent(context); - if (env == nullptr) return; - TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment), - "EnvPromiseHook", env); - - PromiseWrap* wrap = extractPromiseWrap(promise); - if (type == PromiseHookType::kInit || wrap == nullptr) { - bool silent = type != PromiseHookType::kInit; - - // set parent promise's async Id as this promise's triggerAsyncId - if (parent->IsPromise()) { - // parent promise exists, current promise - // is a chained promise, so we set parent promise's id as - // current promise's triggerAsyncId - Local parent_promise = parent.As(); - PromiseWrap* parent_wrap = extractPromiseWrap(parent_promise); - if (parent_wrap == nullptr) { - parent_wrap = PromiseWrap::New(env, parent_promise, true); - if (parent_wrap == nullptr) return; - } - - AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(parent_wrap); - wrap = PromiseWrap::New(env, promise, silent); - } else { - wrap = PromiseWrap::New(env, promise, silent); - } - } - - if (wrap == nullptr) return; - - if (type == PromiseHookType::kBefore) { - env->async_hooks()->push_async_context(wrap->get_async_id(), - wrap->get_trigger_async_id(), wrap->object()); - wrap->EmitTraceEventBefore(); - AsyncWrap::EmitBefore(wrap->env(), wrap->get_async_id()); - } else if (type == PromiseHookType::kAfter) { - wrap->EmitTraceEventAfter(wrap->provider_type(), wrap->get_async_id()); - AsyncWrap::EmitAfter(wrap->env(), wrap->get_async_id()); - if (env->execution_async_id() == wrap->get_async_id()) { - // This condition might not be true if async_hooks was enabled during - // the promise callback execution. - // Popping it off the stack can be skipped in that case, because it is - // known that it would correspond to exactly one call with - // PromiseHookType::kBefore that was not witnessed by the PromiseHook. - env->async_hooks()->pop_async_context(wrap->get_async_id()); - } - } else if (type == PromiseHookType::kResolve) { - AsyncWrap::EmitPromiseResolve(wrap->env(), wrap->get_async_id()); - } -} - static void SetupHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -480,17 +224,6 @@ static void SetupHooks(const FunctionCallbackInfo& args) { #undef SET_HOOK_FN } -static void EnablePromiseHook(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - if (args[0]->IsFunction()) { - env->set_promise_hook_handler(args[0].As()); - args.GetIsolate()->SetPromiseHook(FastPromiseHook); - } else { - args.GetIsolate()->SetPromiseHook(FullPromiseHook); - } -} - static void SetPromiseHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -501,17 +234,6 @@ static void SetPromiseHooks(const FunctionCallbackInfo& args) { args[3]->IsFunction() ? args[3].As() : Local()); } -static void DisablePromiseHook(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - env->set_promise_hook_handler(Local()); - - // The per-Isolate API provides no way of knowing whether there are multiple - // users of the PromiseHook. That hopefully goes away when V8 introduces - // a per-context API. - args.GetIsolate()->SetPromiseHook(nullptr); -} - - class DestroyParam { public: double asyncId; @@ -678,9 +400,7 @@ void AsyncWrap::Initialize(Local target, env->SetMethod(target, "executionAsyncResource", ExecutionAsyncResource); env->SetMethod(target, "clearAsyncIdStack", ClearAsyncIdStack); env->SetMethod(target, "queueDestroyAsyncId", QueueDestroyAsyncId); - env->SetMethod(target, "enablePromiseHook", EnablePromiseHook); env->SetMethod(target, "setPromiseHooks", SetPromiseHooks); - env->SetMethod(target, "disablePromiseHook", DisablePromiseHook); env->SetMethod(target, "registerDestroyHook", RegisterDestroyHook); PropertyAttribute ReadOnlyDontDelete = @@ -764,12 +484,8 @@ void AsyncWrap::Initialize(Local target, FIXED_ONE_BYTE_STRING(env->isolate(), "AsyncWrap"), AsyncWrapObject::GetConstructorTemplate(env) ->GetFunction(env->context()).ToLocalChecked()).Check(); - - // TODO(qard): maybe this should be GetConstructorTemplate instead? - PromiseWrap::Initialize(env); } - AsyncWrap::AsyncWrap(Environment* env, Local object, ProviderType provider, diff --git a/test/addons/async-hooks-promise/binding.cc b/test/addons/async-hooks-promise/binding.cc deleted file mode 100644 index 8fe6b9471bcee5..00000000000000 --- a/test/addons/async-hooks-promise/binding.cc +++ /dev/null @@ -1,41 +0,0 @@ -#include -#include - -namespace { - -using v8::FunctionCallbackInfo; -using v8::Isolate; -using v8::Local; -using v8::Object; -using v8::Promise; -using v8::String; -using v8::Value; - -static void ThrowError(Isolate* isolate, const char* err_msg) { - Local str = String::NewFromOneByte( - isolate, - reinterpret_cast(err_msg)).ToLocalChecked(); - isolate->ThrowException(str); -} - -static void GetPromiseField(const FunctionCallbackInfo& args) { - auto isolate = args.GetIsolate(); - - if (!args[0]->IsPromise()) - return ThrowError(isolate, "arg is not an Promise"); - - auto p = args[0].As(); - if (p->InternalFieldCount() < 1) - return ThrowError(isolate, "Promise has no internal field"); - - auto l = p->GetInternalField(0); - args.GetReturnValue().Set(l); -} - -inline void Initialize(v8::Local binding) { - NODE_SET_METHOD(binding, "getPromiseField", GetPromiseField); -} - -NODE_MODULE(NODE_GYP_MODULE_NAME, Initialize) - -} // anonymous namespace diff --git a/test/addons/async-hooks-promise/binding.gyp b/test/addons/async-hooks-promise/binding.gyp deleted file mode 100644 index 55fbe7050f18e4..00000000000000 --- a/test/addons/async-hooks-promise/binding.gyp +++ /dev/null @@ -1,9 +0,0 @@ -{ - 'targets': [ - { - 'target_name': 'binding', - 'sources': [ 'binding.cc' ], - 'includes': ['../common.gypi'], - } - ] -} diff --git a/test/addons/async-hooks-promise/test.js b/test/addons/async-hooks-promise/test.js deleted file mode 100644 index d38bf9bd978103..00000000000000 --- a/test/addons/async-hooks-promise/test.js +++ /dev/null @@ -1,104 +0,0 @@ -'use strict'; -// Flags: --expose-internals - -const common = require('../../common'); -const assert = require('assert'); -const async_hooks = require('async_hooks'); -const { async_id_symbol, - trigger_async_id_symbol } = require('internal/async_hooks').symbols; -const binding = require(`./build/${common.buildType}/binding`); - -if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { - common.skip('cannot test with env var NODE_TEST_WITH_ASYNC_HOOKS'); - return; -} - -// Baseline to make sure the internal field isn't being set. -assert.strictEqual( - binding.getPromiseField(Promise.resolve(1)), - 0); - -const emptyHook = async_hooks.createHook({}).enable(); - -// Check that no PromiseWrap is created when there are no hook callbacks. -assert.strictEqual( - binding.getPromiseField(Promise.resolve(1)), - 0); - -emptyHook.disable(); - -let lastResource; -let lastAsyncId; -let lastTriggerAsyncId; -const initOnlyHook = async_hooks.createHook({ - init(asyncId, type, triggerAsyncId, resource) { - lastAsyncId = asyncId; - lastTriggerAsyncId = triggerAsyncId; - lastResource = resource; - } -}).enable(); - -// Check that no PromiseWrap is created when only using an init hook. -{ - const promise = Promise.resolve(1); - assert.strictEqual(binding.getPromiseField(promise), 0); - assert.strictEqual(lastResource, promise); - assert.strictEqual(lastAsyncId, promise[async_id_symbol]); - assert.strictEqual(lastTriggerAsyncId, promise[trigger_async_id_symbol]); -} - -initOnlyHook.disable(); - -lastResource = null; -const hookWithDestroy = async_hooks.createHook({ - init(asyncId, type, triggerAsyncId, resource) { - lastAsyncId = asyncId; - lastTriggerAsyncId = triggerAsyncId; - lastResource = resource; - }, - - destroy() { - - } -}).enable(); - -// Check that the internal field returns the same PromiseWrap passed to init(). -{ - const promise = Promise.resolve(1); - const promiseWrap = binding.getPromiseField(promise); - assert.strictEqual(lastResource, promiseWrap); - assert.strictEqual(lastAsyncId, promiseWrap[async_id_symbol]); - assert.strictEqual(lastTriggerAsyncId, promiseWrap[trigger_async_id_symbol]); -} - -hookWithDestroy.disable(); - -// Check that internal fields are no longer being set. This needs to be delayed -// a bit because the `disable()` call only schedules disabling the hook in a -// future microtask. -setImmediate(() => { - assert.strictEqual( - binding.getPromiseField(Promise.resolve(1)), - 0); - - const noDestroyHook = async_hooks.createHook({ - init(asyncId, type, triggerAsyncId, resource) { - lastAsyncId = asyncId; - lastTriggerAsyncId = triggerAsyncId; - lastResource = resource; - }, - - before() {}, - after() {}, - resolve() {} - }).enable(); - - // Check that no PromiseWrap is created when there is no destroy hook. - const promise = Promise.resolve(1); - assert.strictEqual(binding.getPromiseField(promise), 0); - assert.strictEqual(lastResource, promise); - assert.strictEqual(lastAsyncId, promise[async_id_symbol]); - assert.strictEqual(lastTriggerAsyncId, promise[trigger_async_id_symbol]); - - noDestroyHook.disable(); -});