From ef35de1c99fd59c42e03c61007af3d474809c820 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Wed, 19 Apr 2017 13:38:46 -0700 Subject: [PATCH 01/16] async_wrap,src: promise hook integration This change provides unified tracking of asynchronous promise lifecycles for both domains and async hooks. --- src/async-wrap.cc | 205 +++++++++++++------- src/async-wrap.h | 1 + src/node.cc | 61 ------ test/async-hooks/test-promise.js | 51 +++++ test/parallel/test-async-wrap-getasyncid.js | 7 +- 5 files changed, 198 insertions(+), 127 deletions(-) create mode 100644 test/async-hooks/test-promise.js diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 06567d6f7ccf39..e392a6f897c075 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -44,6 +44,8 @@ using v8::Local; using v8::MaybeLocal; using v8::Number; using v8::Object; +using v8::Promise; +using v8::PromiseHookType; using v8::RetainedObjectInfo; using v8::Symbol; using v8::TryCatch; @@ -177,6 +179,135 @@ static void PushBackDestroyId(Environment* env, double id) { } +static bool PreCallbackExecution(AsyncWrap* wrap) { + AsyncHooks* async_hooks = wrap->env()->async_hooks(); + if (wrap->env()->using_domains()) { + Local domain_v = wrap->object()->Get(wrap->env()->domain_string()); + if (domain_v->IsObject()) { + Local domain = domain_v.As(); + if (domain->Get(wrap->env()->disposed_string())->IsTrue()) + return false; + Local enter_v = domain->Get(wrap->env()->enter_string()); + if (enter_v->IsFunction()) { + if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain enter callback threw, please report this"); + } + } + } + } + + 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()); + MaybeLocal ar = fn->Call( + wrap->env()->context(), Undefined(wrap->env()->isolate()), 1, &uid); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(wrap->env()); + FatalException(wrap->env()->isolate(), try_catch); + return false; + } + } + return true; +} + + +static bool PostCallbackExecution(AsyncWrap* wrap) { + AsyncHooks* async_hooks = wrap->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()); + MaybeLocal ar = fn->Call( + wrap->env()->context(), Undefined(wrap->env()->isolate()), 1, &uid); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(wrap->env()); + FatalException(wrap->env()->isolate(), try_catch); + return false; + } + } + + if (wrap->env()->using_domains()) { + Local domain_v = wrap->object()->Get(wrap->env()->domain_string()); + if (domain_v->IsObject()) { + Local domain = domain_v.As(); + if (domain->Get(wrap->env()->disposed_string())->IsTrue()) + return false; + Local exit_v = domain->Get(wrap->env()->exit_string()); + if (exit_v->IsFunction()) { + if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain exit callback threw, please report this"); + } + } + } + } + + return true; +} + +class PromiseWrap : public AsyncWrap { + public: + PromiseWrap(Environment* env, Local object) + : AsyncWrap(env, object, PROVIDER_PROMISE) {} + size_t self_size() const override { return sizeof(*this); } +}; + + +static void GetPromiseDomain(Local name, + const v8::PropertyCallbackInfo& info) { + Local context = info.GetIsolate()->GetCurrentContext(); + Environment* env = Environment::GetCurrent(context); + info.GetReturnValue().Set(Object::Cast(*info.Data())->Get(context, + env->domain_string()).ToLocalChecked()); +} + + +static void PromiseHook(PromiseHookType type, Local promise, + Local parent) { + Local context = promise->CreationContext(); + Environment* env = Environment::GetCurrent(context); + const char async_id_key[] = "__async_wrap"; + const char tag_id_key[] = "__async_wrap_tag"; + if (type == PromiseHookType::kInit) { + // Unfortunately, promises don't have internal fields. Need a surrogate that + // async wrap can wrap. + Local tem = v8::ObjectTemplate::New(env->isolate()); + tem->SetInternalFieldCount(1); + Local obj = tem->NewInstance(context).ToLocalChecked(); + PromiseWrap* wrap = new PromiseWrap(env, obj); + promise->DefineOwnProperty(context, + FIXED_ONE_BYTE_STRING(env->isolate(), async_id_key), + v8::External::New(env->isolate(), wrap), + v8::PropertyAttribute::DontEnum).FromJust(); + promise->DefineOwnProperty(context, + FIXED_ONE_BYTE_STRING(env->isolate(), tag_id_key), + obj, v8::PropertyAttribute::DontEnum).FromJust(); + if (env->in_domain()) { + obj->Set(context, env->domain_string(), + env->domain_array()->Get(context, 0).ToLocalChecked()).FromJust(); + promise->SetAccessor(context, env->domain_string(), + GetPromiseDomain, NULL, obj).FromJust(); + } + } else if (type == PromiseHookType::kResolve) { + // TODO(matthewloring): need to expose this through the async hooks api. + } + Local external_wrap = promise->Get(context, FIXED_ONE_BYTE_STRING(env->isolate(), + async_id_key)).ToLocalChecked(); + PromiseWrap* wrap = + static_cast(v8::External::Cast(*external_wrap)->Value()); + if (type == PromiseHookType::kBefore) { + PreCallbackExecution(wrap); + } else if (type == PromiseHookType::kAfter) { + PostCallbackExecution(wrap); + } +} + + static void SetupHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -201,6 +332,7 @@ static void SetupHooks(const FunctionCallbackInfo& args) { SET_HOOK_FN(before); SET_HOOK_FN(after); SET_HOOK_FN(destroy); + env->isolate()->SetPromiseHook(PromiseHook); #undef SET_HOOK_FN } @@ -416,87 +548,30 @@ Local AsyncWrap::MakeCallback(const Local cb, Local* argv) { CHECK(env()->context() == env()->isolate()->GetCurrentContext()); - AsyncHooks* async_hooks = env()->async_hooks(); - Local context = object(); - Local domain; - Local uid; - bool has_domain = false; - Environment::AsyncCallbackScope callback_scope(env()); - if (env()->using_domains()) { - Local domain_v = context->Get(env()->domain_string()); - has_domain = domain_v->IsObject(); - if (has_domain) { - domain = domain_v.As(); - if (domain->Get(env()->disposed_string())->IsTrue()) - return Local(); - } - } - - 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::AsyncWrap::MakeCallback", - "domain enter callback threw, please report this"); - } - } - } - - // Want currentId() to return the correct value from the callbacks. - AsyncHooks::ExecScope exec_scope(env(), get_id(), get_trigger_id()); + Environment::AsyncHooks::ExecScope exec_scope(env(), + get_id(), + get_trigger_id()); - if (async_hooks->fields()[AsyncHooks::kBefore] > 0) { - uid = Number::New(env()->isolate(), get_id()); - Local fn = env()->async_hooks_before_function(); - TryCatch try_catch(env()->isolate()); - MaybeLocal ar = fn->Call( - env()->context(), Undefined(env()->isolate()), 1, &uid); - if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(env()); - FatalException(env()->isolate(), try_catch); - return Local(); - } + if (!PreCallbackExecution(this)) { + return Local(); } // Finally... Get to running the user's callback. - MaybeLocal ret = cb->Call(env()->context(), context, argc, argv); + MaybeLocal ret = cb->Call(env()->context(), object(), argc, argv); Local ret_v; if (!ret.ToLocal(&ret_v)) { return Local(); } - // If the callback failed then the after() hooks will be called at the end - // of _fatalException(). - if (async_hooks->fields()[AsyncHooks::kAfter] > 0) { - if (uid.IsEmpty()) - uid = Number::New(env()->isolate(), get_id()); - Local fn = env()->async_hooks_after_function(); - TryCatch try_catch(env()->isolate()); - MaybeLocal ar = fn->Call( - env()->context(), Undefined(env()->isolate()), 1, &uid); - if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(env()); - FatalException(env()->isolate(), try_catch); - return Local(); - } + if (!PostCallbackExecution(this)) { + return Local(); } - // The execution scope of the id and trigger_id only go this far. exec_scope.Dispose(); - 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::AsyncWrap::MakeCallback", - "domain exit callback threw, please report this"); - } - } - } - if (callback_scope.in_makecallback()) { return ret_v; } diff --git a/src/async-wrap.h b/src/async-wrap.h index 8a93e838786297..0d1bdd9b92ab9f 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -47,6 +47,7 @@ namespace node { V(PIPECONNECTWRAP) \ V(PIPEWRAP) \ V(PROCESSWRAP) \ + V(PROMISE) \ V(QUERYWRAP) \ V(RANDOMBYTESREQUEST) \ V(SHUTDOWNWRAP) \ diff --git a/src/node.cc b/src/node.cc index 770c68d57520d6..5333bfe195859b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -143,7 +143,6 @@ using v8::Number; using v8::Object; using v8::ObjectTemplate; using v8::Promise; -using v8::PromiseHookType; using v8::PromiseRejectMessage; using v8::PropertyCallbackInfo; using v8::ScriptOrigin; @@ -1117,58 +1116,6 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { } -void DomainPromiseHook(PromiseHookType type, - Local promise, - Local parent, - void* arg) { - Environment* env = static_cast(arg); - Local context = env->context(); - - if (type == PromiseHookType::kResolve) return; - if (type == PromiseHookType::kInit && env->in_domain()) { - promise->Set(context, - env->domain_string(), - env->domain_array()->Get(context, - 0).ToLocalChecked()).FromJust(); - return; - } - - // Loosely based on node::MakeCallback(). - Local domain_v = - promise->Get(context, env->domain_string()).ToLocalChecked(); - if (!domain_v->IsObject()) - return; - - Local domain = domain_v.As(); - if (domain->Get(context, env->disposed_string()) - .ToLocalChecked()->IsTrue()) { - return; - } - - if (type == PromiseHookType::kBefore) { - Local enter_v = - domain->Get(context, env->enter_string()).ToLocalChecked(); - if (enter_v->IsFunction()) { - if (enter_v.As()->Call(context, domain, 0, nullptr).IsEmpty()) { - FatalError("node::PromiseHook", - "domain enter callback threw, please report this " - "as a bug in Node.js"); - } - } - } else { - Local exit_v = - domain->Get(context, env->exit_string()).ToLocalChecked(); - if (exit_v->IsFunction()) { - if (exit_v.As()->Call(context, domain, 0, nullptr).IsEmpty()) { - FatalError("node::MakeCallback", - "domain exit callback threw, please report this " - "as a bug in Node.js"); - } - } - } -} - - void SetupDomainUse(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1208,8 +1155,6 @@ void SetupDomainUse(const FunctionCallbackInfo& args) { Local array_buffer = ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count); - env->AddPromiseHook(DomainPromiseHook, static_cast(env)); - args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count)); } @@ -1291,12 +1236,6 @@ void SetupPromises(const FunctionCallbackInfo& args) { } // anonymous namespace -void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { - Environment* env = Environment::GetCurrent(isolate); - env->AddPromiseHook(fn, arg); -} - - Local MakeCallback(Environment* env, Local recv, const Local callback, diff --git a/test/async-hooks/test-promise.js b/test/async-hooks/test-promise.js new file mode 100644 index 00000000000000..afc529a9d315de --- /dev/null +++ b/test/async-hooks/test-promise.js @@ -0,0 +1,51 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const initHooks = require('./init-hooks'); +const { checkInvocations } = require('./hook-checks'); + +const hooks = initHooks(); + +hooks.enable(); + +const p = (new Promise(common.mustCall(executor))); +p.then(afterresolution); + +function executor(resolve, reject) { + const as = hooks.activitiesOfTypes('PROMISE'); + assert.strictEqual(as.length, 1, 'one activities'); + const a = as[0]; + checkInvocations(a, { init: 1 }, 'while in promise executor'); + resolve(5); +} + +function afterresolution(val) { + assert.strictEqual(val, 5); + const as = hooks.activitiesOfTypes('PROMISE'); + assert.strictEqual(as.length, 2, 'two activities'); + checkInvocations(as[0], { init: 1 }, 'after resolution parent promise'); + checkInvocations(as[1], { init: 1, before: 1 }, + 'after resolution child promise'); +} + +process.on('exit', onexit); +function onexit() { + hooks.disable(); + hooks.sanityCheck('PROMISE'); + + const as = hooks.activitiesOfTypes('PROMISE'); + assert.strictEqual(as.length, 2, 'two activities'); + + const a0 = as[0]; + assert.strictEqual(a0.type, 'PROMISE', 'promise request'); + assert.strictEqual(typeof a0.uid, 'number', 'uid is a number'); + assert.strictEqual(a0.triggerId, 1, 'parent uid 1'); + checkInvocations(a0, { init: 1 }, 'when process exits'); + + const a1 = as[1]; + assert.strictEqual(a1.type, 'PROMISE', 'promise request'); + assert.strictEqual(typeof a1.uid, 'number', 'uid is a number'); + assert.strictEqual(a1.triggerId, 1, 'parent uid 1'); + checkInvocations(a1, { init: 1, before: 1, after: 1 }, 'when process exits'); +} diff --git a/test/parallel/test-async-wrap-getasyncid.js b/test/parallel/test-async-wrap-getasyncid.js index 98aa3c857a8b18..7052792ee8b72a 100644 --- a/test/parallel/test-async-wrap-getasyncid.js +++ b/test/parallel/test-async-wrap-getasyncid.js @@ -66,6 +66,12 @@ function testInitialized(req, ctor_name) { } +{ + // TODO: determine the rigth way to expose promise wrap. + new Promise((res) => res(5)); +} + + if (common.hasCrypto) { const tls = require('tls'); // SecurePair @@ -137,7 +143,6 @@ if (common.hasCrypto) { testInitialized(new Process(), 'Process'); } - { const Signal = process.binding('signal_wrap').Signal; testInitialized(new Signal(), 'Signal'); From 563c1b0c973e687fcc7fbc72ca830437cd4c358f Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Mon, 15 May 2017 13:48:18 -0700 Subject: [PATCH 02/16] Reintroduce promise hook sharing --- src/async-wrap.cc | 4 ++-- src/node.cc | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index e392a6f897c075..7d6ed6b6e8e25a 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -268,7 +268,7 @@ static void GetPromiseDomain(Local name, static void PromiseHook(PromiseHookType type, Local promise, - Local parent) { + Local parent, void* arg) { Local context = promise->CreationContext(); Environment* env = Environment::GetCurrent(context); const char async_id_key[] = "__async_wrap"; @@ -332,7 +332,7 @@ static void SetupHooks(const FunctionCallbackInfo& args) { SET_HOOK_FN(before); SET_HOOK_FN(after); SET_HOOK_FN(destroy); - env->isolate()->SetPromiseHook(PromiseHook); + env->AddPromiseHook(PromiseHook, nullptr); #undef SET_HOOK_FN } diff --git a/src/node.cc b/src/node.cc index 5333bfe195859b..f50cc817d5c0ae 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1236,6 +1236,12 @@ void SetupPromises(const FunctionCallbackInfo& args) { } // anonymous namespace +void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->AddPromiseHook(fn, arg); +} + + Local MakeCallback(Environment* env, Local recv, const Local callback, From 4aaadc0f9ba3abb36eff96d4f61bdd084dcc5e03 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Mon, 15 May 2017 15:13:31 -0700 Subject: [PATCH 03/16] Cleanup --- src/async-wrap.cc | 13 +++++++------ src/env.h | 2 ++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 7d6ed6b6e8e25a..810061009c241b 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -262,8 +262,9 @@ static void GetPromiseDomain(Local name, const v8::PropertyCallbackInfo& info) { Local context = info.GetIsolate()->GetCurrentContext(); Environment* env = Environment::GetCurrent(context); - info.GetReturnValue().Set(Object::Cast(*info.Data())->Get(context, - env->domain_string()).ToLocalChecked()); + info.GetReturnValue().Set( + info.Data().As()->Get(context, + env->domain_string()).ToLocalChecked()); } @@ -281,11 +282,11 @@ static void PromiseHook(PromiseHookType type, Local promise, Local obj = tem->NewInstance(context).ToLocalChecked(); PromiseWrap* wrap = new PromiseWrap(env, obj); promise->DefineOwnProperty(context, - FIXED_ONE_BYTE_STRING(env->isolate(), async_id_key), + env->promise_async_id(), v8::External::New(env->isolate(), wrap), v8::PropertyAttribute::DontEnum).FromJust(); promise->DefineOwnProperty(context, - FIXED_ONE_BYTE_STRING(env->isolate(), tag_id_key), + env->promise_async_tag(), obj, v8::PropertyAttribute::DontEnum).FromJust(); if (env->in_domain()) { obj->Set(context, env->domain_string(), @@ -296,8 +297,8 @@ static void PromiseHook(PromiseHookType type, Local promise, } else if (type == PromiseHookType::kResolve) { // TODO(matthewloring): need to expose this through the async hooks api. } - Local external_wrap = promise->Get(context, FIXED_ONE_BYTE_STRING(env->isolate(), - async_id_key)).ToLocalChecked(); + Local external_wrap = + promise->Get(context, env->promise_async_id()).ToLocalChecked(); PromiseWrap* wrap = static_cast(v8::External::Cast(*external_wrap)->Value()); if (type == PromiseHookType::kBefore) { diff --git a/src/env.h b/src/env.h index 1a8157949a13f4..ae46e41e9515d5 100644 --- a/src/env.h +++ b/src/env.h @@ -195,6 +195,8 @@ namespace node { V(preference_string, "preference") \ V(priority_string, "priority") \ V(produce_cached_data_string, "produceCachedData") \ + V(promise_async_id, "_promise_async_wrap") \ + V(promise_async_tag, "_promise_async_wrap_tag") \ V(raw_string, "raw") \ V(read_host_object_string, "_readHostObject") \ V(readable_string, "readable") \ From 70f9675de33352dba9b4faef66fa6c1aeabd6042 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Mon, 22 May 2017 17:16:14 -0700 Subject: [PATCH 04/16] Clean up comments --- test/parallel/test-async-wrap-getasyncid.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-async-wrap-getasyncid.js b/test/parallel/test-async-wrap-getasyncid.js index 7052792ee8b72a..2b5774f34b4127 100644 --- a/test/parallel/test-async-wrap-getasyncid.js +++ b/test/parallel/test-async-wrap-getasyncid.js @@ -67,7 +67,8 @@ function testInitialized(req, ctor_name) { { - // TODO: determine the rigth way to expose promise wrap. + // We don't want to expose getAsyncId for promises but we need to construct one + // so that the cooresponding provider type is removed from the providers list. new Promise((res) => res(5)); } From 0afbcfc0ef649801259d12595655f722270d7a92 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 May 2017 16:13:13 +0200 Subject: [PATCH 05/16] remove unused variables --- src/async-wrap.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 810061009c241b..7451521f6a7d81 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -272,8 +272,6 @@ static void PromiseHook(PromiseHookType type, Local promise, Local parent, void* arg) { Local context = promise->CreationContext(); Environment* env = Environment::GetCurrent(context); - const char async_id_key[] = "__async_wrap"; - const char tag_id_key[] = "__async_wrap_tag"; if (type == PromiseHookType::kInit) { // Unfortunately, promises don't have internal fields. Need a surrogate that // async wrap can wrap. From 833ad39133d398a1f84336052cbd13adf3a6ef4b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 May 2017 16:15:46 +0200 Subject: [PATCH 06/16] use .As<>() for casting --- src/async-wrap.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 7451521f6a7d81..1fc4dab0311bb5 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -298,7 +298,7 @@ static void PromiseHook(PromiseHookType type, Local promise, Local external_wrap = promise->Get(context, env->promise_async_id()).ToLocalChecked(); PromiseWrap* wrap = - static_cast(v8::External::Cast(*external_wrap)->Value()); + static_cast(external_wrap.As()->Value()); if (type == PromiseHookType::kBefore) { PreCallbackExecution(wrap); } else if (type == PromiseHookType::kAfter) { From 5717d75e97f4699b2c7eb706b10a129cc11da2f2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 May 2017 16:17:21 +0200 Subject: [PATCH 07/16] align arguments (trevnorris review) --- src/async-wrap.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 1fc4dab0311bb5..5f681c797036c5 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -191,7 +191,7 @@ static bool PreCallbackExecution(AsyncWrap* wrap) { if (enter_v->IsFunction()) { if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { FatalError("node::AsyncWrap::MakeCallback", - "domain enter callback threw, please report this"); + "domain enter callback threw, please report this"); } } } From 34ee31f1272a56c5f1ce67c9277ea0c579e92663 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 May 2017 16:17:49 +0200 Subject: [PATCH 08/16] add CHECK_NE() (trevnorris review) --- src/async-wrap.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 5f681c797036c5..e6c33d594cfeae 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -299,6 +299,7 @@ static void PromiseHook(PromiseHookType type, Local promise, promise->Get(context, env->promise_async_id()).ToLocalChecked(); PromiseWrap* wrap = static_cast(external_wrap.As()->Value()); + CHECK_NE(wrap, nullptr); if (type == PromiseHookType::kBefore) { PreCallbackExecution(wrap); } else if (type == PromiseHookType::kAfter) { From e0c194b8e73ddea75449d850d17b6723721561b8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 May 2017 17:05:40 +0200 Subject: [PATCH 09/16] re-untangle domain/async_hooks promise support --- src/async-wrap.cc | 84 ++++++++++++++++++++++++++++------------------- src/async-wrap.h | 3 ++ src/node.cc | 26 +++++++++++++++ 3 files changed, 80 insertions(+), 33 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index e6c33d594cfeae..dcef462b8e9483 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -179,23 +179,50 @@ static void PushBackDestroyId(Environment* env, double id) { } -static bool PreCallbackExecution(AsyncWrap* wrap) { - AsyncHooks* async_hooks = wrap->env()->async_hooks(); - if (wrap->env()->using_domains()) { - Local domain_v = wrap->object()->Get(wrap->env()->domain_string()); - if (domain_v->IsObject()) { - Local domain = domain_v.As(); - if (domain->Get(wrap->env()->disposed_string())->IsTrue()) - return false; - Local enter_v = domain->Get(wrap->env()->enter_string()); - if (enter_v->IsFunction()) { - if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { - FatalError("node::AsyncWrap::MakeCallback", - "domain enter callback threw, please report this"); - } +bool DomainEnter(Environment* env, Local object) { + Local domain_v = object->Get(env->domain_string()); + if (domain_v->IsObject()) { + Local domain = domain_v.As(); + if (domain->Get(env->disposed_string())->IsTrue()) + return true; + Local enter_v = domain->Get(env->enter_string()); + if (enter_v->IsFunction()) { + if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain enter callback threw, please report this"); } } } + return false; +} + + +bool DomainExit(Environment* env, v8::Local object) { + Local domain_v = object->Get(env->domain_string()); + if (domain_v->IsObject()) { + Local domain = domain_v.As(); + if (domain->Get(env->disposed_string())->IsTrue()) + return true; + Local exit_v = domain->Get(env->exit_string()); + if (exit_v->IsFunction()) { + if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain exit callback threw, please report this"); + } + } + } + return false; +} + + +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; + } if (async_hooks->fields()[AsyncHooks::kBefore] > 0) { Local uid = Number::New(wrap->env()->isolate(), wrap->get_id()); @@ -209,11 +236,12 @@ static bool PreCallbackExecution(AsyncWrap* wrap) { return false; } } + return true; } -static bool PostCallbackExecution(AsyncWrap* wrap) { +static bool PostCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) { AsyncHooks* async_hooks = wrap->env()->async_hooks(); // If the callback failed then the after() hooks will be called at the end @@ -231,20 +259,10 @@ static bool PostCallbackExecution(AsyncWrap* wrap) { } } - if (wrap->env()->using_domains()) { - Local domain_v = wrap->object()->Get(wrap->env()->domain_string()); - if (domain_v->IsObject()) { - Local domain = domain_v.As(); - if (domain->Get(wrap->env()->disposed_string())->IsTrue()) - return false; - Local exit_v = domain->Get(wrap->env()->exit_string()); - if (exit_v->IsFunction()) { - if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { - FatalError("node::AsyncWrap::MakeCallback", - "domain exit callback threw, please report this"); - } - } - } + if (wrap->env()->using_domains() && run_domain_cbs) { + bool is_disposed = DomainExit(wrap->env(), wrap->object()); + if (is_disposed) + return false; } return true; @@ -301,9 +319,9 @@ static void PromiseHook(PromiseHookType type, Local promise, static_cast(external_wrap.As()->Value()); CHECK_NE(wrap, nullptr); if (type == PromiseHookType::kBefore) { - PreCallbackExecution(wrap); + PreCallbackExecution(wrap, false); } else if (type == PromiseHookType::kAfter) { - PostCallbackExecution(wrap); + PostCallbackExecution(wrap, false); } } @@ -554,7 +572,7 @@ Local AsyncWrap::MakeCallback(const Local cb, get_id(), get_trigger_id()); - if (!PreCallbackExecution(this)) { + if (!PreCallbackExecution(this, true)) { return Local(); } @@ -566,7 +584,7 @@ Local AsyncWrap::MakeCallback(const Local cb, return Local(); } - if (!PostCallbackExecution(this)) { + if (!PostCallbackExecution(this, true)) { return Local(); } diff --git a/src/async-wrap.h b/src/async-wrap.h index 0d1bdd9b92ab9f..0ce837d5c3f891 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -124,6 +124,9 @@ class AsyncWrap : public BaseObject { void LoadAsyncWrapperInfo(Environment* env); +bool DomainEnter(Environment* env, v8::Local object); +bool DomainExit(Environment* env, v8::Local object); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node.cc b/src/node.cc index f50cc817d5c0ae..7319bf257697b8 100644 --- a/src/node.cc +++ b/src/node.cc @@ -143,6 +143,7 @@ using v8::Number; using v8::Object; using v8::ObjectTemplate; using v8::Promise; +using v8::PromiseHookType; using v8::PromiseRejectMessage; using v8::PropertyCallbackInfo; using v8::ScriptOrigin; @@ -1116,6 +1117,29 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { } +void DomainPromiseHook(PromiseHookType type, + Local promise, + Local parent, + void* arg) { + Environment* env = static_cast(arg); + Local context = env->context(); + + if (type == PromiseHookType::kInit && env->in_domain()) { + promise->Set(context, + env->domain_string(), + env->domain_array()->Get(context, + 0).ToLocalChecked()).FromJust(); + return; + } + + if (type == PromiseHookType::kBefore) { + DomainEnter(env, promise); + } else if (type == PromiseHookType::kAfter) { + DomainExit(env, promise); + } +} + + void SetupDomainUse(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1155,6 +1179,8 @@ void SetupDomainUse(const FunctionCallbackInfo& args) { Local array_buffer = ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count); + env->AddPromiseHook(DomainPromiseHook, static_cast(env)); + args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count)); } From e1c11e73294286d205a90bb5fe0fc8c272556ab2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 May 2017 17:14:21 +0200 Subject: [PATCH 10/16] async_hooks: only set up hooks if used --- lib/async_hooks.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 867b5eb52da14d..49f8fa5becf2db 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -49,12 +49,7 @@ const before_symbol = Symbol('before'); const after_symbol = Symbol('after'); const destroy_symbol = Symbol('destroy'); -// Setup the callbacks that node::AsyncWrap will call when there are hooks to -// process. They use the same functions as the JS embedder API. -async_wrap.setupHooks({ init, - before: emitBeforeN, - after: emitAfterN, - destroy: emitDestroyN }); +let setupHooksCalled = false; // Used to fatally abort the process if a callback throws. function fatalError(e) { @@ -103,6 +98,16 @@ class AsyncHook { if (hooks_array.includes(this)) return; + if (!setupHooksCalled) { + setupHooksCalled = true; + // Setup the callbacks that node::AsyncWrap will call when there are + // hooks to process. They use the same functions as the JS embedder API. + async_wrap.setupHooks({ init, + before: emitBeforeN, + after: emitAfterN, + destroy: emitDestroyN }); + } + // createHook() has already enforced that the callbacks are all functions, // so here simply increment the count of whether each callbacks exists or // not. From d47e97ac3d626b9476cb0d671f9cd74420420ed7 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Tue, 23 May 2017 15:40:36 -0700 Subject: [PATCH 11/16] Clarify use of additional object to track promise lifespan. --- src/async-wrap.cc | 7 +++++-- src/env.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index dcef462b8e9483..8a2699f40717ba 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -298,9 +298,12 @@ static void PromiseHook(PromiseHookType type, Local promise, Local obj = tem->NewInstance(context).ToLocalChecked(); PromiseWrap* wrap = new PromiseWrap(env, obj); promise->DefineOwnProperty(context, - env->promise_async_id(), + env->promise_wrap(), v8::External::New(env->isolate(), wrap), v8::PropertyAttribute::DontEnum).FromJust(); + // The async tag will be destroyed at the same time as the promise as the only + // reference to it is held by the promise. This allows the promise wrap instance + // to be notified when the promise is destroyed. promise->DefineOwnProperty(context, env->promise_async_tag(), obj, v8::PropertyAttribute::DontEnum).FromJust(); @@ -314,7 +317,7 @@ static void PromiseHook(PromiseHookType type, Local promise, // TODO(matthewloring): need to expose this through the async hooks api. } Local external_wrap = - promise->Get(context, env->promise_async_id()).ToLocalChecked(); + promise->Get(context, env->promise_wrap()).ToLocalChecked(); PromiseWrap* wrap = static_cast(external_wrap.As()->Value()); CHECK_NE(wrap, nullptr); diff --git a/src/env.h b/src/env.h index ae46e41e9515d5..958329ebd0158b 100644 --- a/src/env.h +++ b/src/env.h @@ -195,7 +195,7 @@ namespace node { V(preference_string, "preference") \ V(priority_string, "priority") \ V(produce_cached_data_string, "produceCachedData") \ - V(promise_async_id, "_promise_async_wrap") \ + V(promise_wrap, "_promise_async_wrap") \ V(promise_async_tag, "_promise_async_wrap_tag") \ V(raw_string, "raw") \ V(read_host_object_string, "_readHostObject") \ From 012171a86db580ae46c6e1e22ebf514e295b4390 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Tue, 23 May 2017 15:43:06 -0700 Subject: [PATCH 12/16] Clarifying comment about destroy hooks in test-promise.js --- test/async-hooks/test-promise.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/async-hooks/test-promise.js b/test/async-hooks/test-promise.js index afc529a9d315de..30cf94d28b15ab 100644 --- a/test/async-hooks/test-promise.js +++ b/test/async-hooks/test-promise.js @@ -47,5 +47,6 @@ function onexit() { assert.strictEqual(a1.type, 'PROMISE', 'promise request'); assert.strictEqual(typeof a1.uid, 'number', 'uid is a number'); assert.strictEqual(a1.triggerId, 1, 'parent uid 1'); + // We expect a destroy hook as well but we cannot guarentee predictable gc. checkInvocations(a1, { init: 1, before: 1, after: 1 }, 'when process exits'); } From 4e38bda556ac3f53da872854d0d886186b30103a Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Tue, 23 May 2017 15:50:37 -0700 Subject: [PATCH 13/16] Remove redundant domain tracking --- src/async-wrap.cc | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 8a2699f40717ba..101c74d7e63894 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -276,16 +276,6 @@ class PromiseWrap : public AsyncWrap { }; -static void GetPromiseDomain(Local name, - const v8::PropertyCallbackInfo& info) { - Local context = info.GetIsolate()->GetCurrentContext(); - Environment* env = Environment::GetCurrent(context); - info.GetReturnValue().Set( - info.Data().As()->Get(context, - env->domain_string()).ToLocalChecked()); -} - - static void PromiseHook(PromiseHookType type, Local promise, Local parent, void* arg) { Local context = promise->CreationContext(); @@ -307,12 +297,6 @@ static void PromiseHook(PromiseHookType type, Local promise, promise->DefineOwnProperty(context, env->promise_async_tag(), obj, v8::PropertyAttribute::DontEnum).FromJust(); - if (env->in_domain()) { - obj->Set(context, env->domain_string(), - env->domain_array()->Get(context, 0).ToLocalChecked()).FromJust(); - promise->SetAccessor(context, env->domain_string(), - GetPromiseDomain, NULL, obj).FromJust(); - } } else if (type == PromiseHookType::kResolve) { // TODO(matthewloring): need to expose this through the async hooks api. } From 13313bbe3a4e6bd26bec760b66ce510e37557009 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Tue, 23 May 2017 18:03:13 -0700 Subject: [PATCH 14/16] Fix lint errors --- src/async-wrap.cc | 6 +++--- test/parallel/test-async-wrap-getasyncid.js | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 101c74d7e63894..a01e91f81e5762 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -291,9 +291,9 @@ static void PromiseHook(PromiseHookType type, Local promise, env->promise_wrap(), v8::External::New(env->isolate(), wrap), v8::PropertyAttribute::DontEnum).FromJust(); - // The async tag will be destroyed at the same time as the promise as the only - // reference to it is held by the promise. This allows the promise wrap instance - // to be notified when the promise is destroyed. + // The async tag will be destroyed at the same time as the promise as the + // only reference to it is held by the promise. This allows the promise + // wrap instance to be notified when the promise is destroyed. promise->DefineOwnProperty(context, env->promise_async_tag(), obj, v8::PropertyAttribute::DontEnum).FromJust(); diff --git a/test/parallel/test-async-wrap-getasyncid.js b/test/parallel/test-async-wrap-getasyncid.js index 2b5774f34b4127..6b9d33b8ca25c7 100644 --- a/test/parallel/test-async-wrap-getasyncid.js +++ b/test/parallel/test-async-wrap-getasyncid.js @@ -67,8 +67,9 @@ function testInitialized(req, ctor_name) { { - // We don't want to expose getAsyncId for promises but we need to construct one - // so that the cooresponding provider type is removed from the providers list. + // We don't want to expose getAsyncId for promises but we need to construct + // one so that the cooresponding provider type is removed from the + // providers list. new Promise((res) => res(5)); } From 9b9c3103310e2e3da4c6b029401a3de141f29bd9 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Wed, 24 May 2017 09:43:32 -0700 Subject: [PATCH 15/16] Performance improvements --- src/async-wrap.cc | 16 +++++++++++----- src/env.h | 1 + 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index a01e91f81e5762..6351deb9a53968 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -283,20 +283,21 @@ static void PromiseHook(PromiseHookType type, Local promise, if (type == PromiseHookType::kInit) { // Unfortunately, promises don't have internal fields. Need a surrogate that // async wrap can wrap. - Local tem = v8::ObjectTemplate::New(env->isolate()); - tem->SetInternalFieldCount(1); - Local obj = tem->NewInstance(context).ToLocalChecked(); + Local obj = + env->async_hooks_promise_object()->NewInstance(context).ToLocalChecked(); PromiseWrap* wrap = new PromiseWrap(env, obj); + v8::PropertyAttribute hidden = + static_cast(v8::ReadOnly | v8::DontDelete | v8::DontEnum); promise->DefineOwnProperty(context, env->promise_wrap(), v8::External::New(env->isolate(), wrap), - v8::PropertyAttribute::DontEnum).FromJust(); + hidden).FromJust(); // The async tag will be destroyed at the same time as the promise as the // only reference to it is held by the promise. This allows the promise // wrap instance to be notified when the promise is destroyed. promise->DefineOwnProperty(context, env->promise_async_tag(), - obj, v8::PropertyAttribute::DontEnum).FromJust(); + obj, hidden).FromJust(); } else if (type == PromiseHookType::kResolve) { // TODO(matthewloring): need to expose this through the async hooks api. } @@ -399,6 +400,11 @@ void AsyncWrap::Initialize(Local target, env->SetMethod(target, "clearIdStack", ClearIdStack); env->SetMethod(target, "addIdToDestroyList", QueueDestroyId); + Local promise_object_template = + v8::ObjectTemplate::New(env->isolate()); + promise_object_template->SetInternalFieldCount(1); + env->set_async_hooks_promise_object(promise_object_template); + v8::PropertyAttribute ReadOnlyDontDelete = static_cast(v8::ReadOnly | v8::DontDelete); diff --git a/src/env.h b/src/env.h index 958329ebd0158b..09b455579ea61e 100644 --- a/src/env.h +++ b/src/env.h @@ -258,6 +258,7 @@ namespace node { V(async_hooks_init_function, v8::Function) \ V(async_hooks_before_function, v8::Function) \ V(async_hooks_after_function, v8::Function) \ + V(async_hooks_promise_object, v8::ObjectTemplate) \ V(binding_cache_object, v8::Object) \ V(buffer_constructor_function, v8::Function) \ V(buffer_prototype_object, v8::Object) \ From 81ff7f61926008206196968647d7db77e70b5d78 Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Wed, 24 May 2017 11:37:38 -0700 Subject: [PATCH 16/16] Appease linter --- src/async-wrap.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 6351deb9a53968..d3776c73334b49 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -287,7 +287,9 @@ static void PromiseHook(PromiseHookType type, Local promise, env->async_hooks_promise_object()->NewInstance(context).ToLocalChecked(); PromiseWrap* wrap = new PromiseWrap(env, obj); v8::PropertyAttribute hidden = - static_cast(v8::ReadOnly | v8::DontDelete | v8::DontEnum); + static_cast(v8::ReadOnly + | v8::DontDelete + | v8::DontEnum); promise->DefineOwnProperty(context, env->promise_wrap(), v8::External::New(env->isolate(), wrap),