From 3f5159017f536302d916a6533770b00463d98ea8 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 4 May 2015 15:56:40 -0600 Subject: [PATCH 1/4] async-wrap: don't call init callback unnecessarily Some calls to ReqWrap would get through the initial check and allow the init callback to run, even though the callback had not been used on the parent. Fix by explicitly checking if the parent has a queue. Also change the name of the check, and internal field of AsyncHooks. Other names were confusing. --- src/async-wrap-inl.h | 8 +++++++- src/env-inl.h | 12 ++++++++---- src/env.h | 7 ++++--- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index b08f791c2e7635..ba84af19c02d23 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -21,7 +21,13 @@ inline AsyncWrap::AsyncWrap(Environment* env, // Check user controlled flag to see if the init callback should run. if (!env->using_asyncwrap()) return; - if (!env->call_async_init_hook() && parent == nullptr) + + // If callback hooks have not been enabled, and there is no parent, return. + if (!env->async_wrap_callbacks_enabled() && parent == nullptr) + return; + + // If callback hooks have not been enabled and parent has no queue, return. + if (!env->async_wrap_callbacks_enabled() && !parent->has_async_queue()) return; // TODO(trevnorris): Until it's verified all passed object's are not weak, diff --git a/src/env-inl.h b/src/env-inl.h index 237da7159d1ecf..a0815acaccb926 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -66,8 +66,12 @@ inline int Environment::AsyncHooks::fields_count() const { return kFieldsCount; } -inline bool Environment::AsyncHooks::call_init_hook() { - return fields_[kCallInitHook] != 0; +inline bool Environment::AsyncHooks::callbacks_enabled() { + return fields_[kEnableCallbacks] != 0; +} + +inline void Environment::AsyncHooks::set_enable_callbacks(uint32_t flag) { + fields_[kEnableCallbacks] = flag; } inline Environment::DomainFlag::DomainFlag() { @@ -214,9 +218,9 @@ inline v8::Isolate* Environment::isolate() const { return isolate_; } -inline bool Environment::call_async_init_hook() const { +inline bool Environment::async_wrap_callbacks_enabled() const { // The const_cast is okay, it doesn't violate conceptual const-ness. - return const_cast(this)->async_hooks()->call_init_hook(); + return const_cast(this)->async_hooks()->callbacks_enabled(); } inline bool Environment::in_domain() const { diff --git a/src/env.h b/src/env.h index 8a926340673c6e..e327786e36b907 100644 --- a/src/env.h +++ b/src/env.h @@ -269,7 +269,8 @@ class Environment { public: inline uint32_t* fields(); inline int fields_count() const; - inline bool call_init_hook(); + inline bool callbacks_enabled(); + inline void set_enable_callbacks(uint32_t flag); private: friend class Environment; // So we can call the constructor. @@ -277,7 +278,7 @@ class Environment { enum Fields { // Set this to not zero if the init hook should be called. - kCallInitHook, + kEnableCallbacks, kFieldsCount }; @@ -374,7 +375,7 @@ class Environment { inline v8::Isolate* isolate() const; inline uv_loop_t* event_loop() const; - inline bool call_async_init_hook() const; + inline bool async_wrap_callbacks_enabled() const; inline bool in_domain() const; inline uint32_t watched_providers() const; From cc0385e022515bb99f67140c8b3c273b23d2cc1f Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 4 May 2015 15:59:07 -0600 Subject: [PATCH 2/4] async-wrap: pass PROVIDER as first arg to init Allow the init callback to see the PROVIDER type easily by being able to compare the flag with the list of providers on the exported async_wrap object. --- src/async-wrap-inl.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index ba84af19c02d23..46d0e19e536eed 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -48,7 +48,8 @@ inline AsyncWrap::AsyncWrap(Environment* env, FatalError("node::AsyncWrap::AsyncWrap", "parent pre hook threw"); } - env->async_hooks_init_function()->Call(object, 0, nullptr); + v8::Local n = v8::Int32::New(env->isolate(), provider); + env->async_hooks_init_function()->Call(object, 1, &n); if (try_catch.HasCaught()) FatalError("node::AsyncWrap::AsyncWrap", "init hook threw"); From 7740d5e80802e9181b49a877ff69fed416040e5b Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 4 May 2015 16:00:37 -0600 Subject: [PATCH 3/4] async-wrap: set flags using functions Setting flags using cryptic numeric object fields is confusing. Instead use much simpler .enable()/.disable() calls on the async_wrap object. --- src/async-wrap.cc | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 5710c43146060b..3cb2cb4af1d477 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -23,32 +23,28 @@ using v8::kExternalUint32Array; namespace node { +static void EnableHooksJS(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + env->async_hooks()->set_enable_callbacks(1); +} + + +static void DisableHooksJS(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + env->async_hooks()->set_enable_callbacks(0); +} + + static void SetupHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args.GetIsolate()); - CHECK(args[0]->IsObject()); + CHECK(args[0]->IsFunction()); CHECK(args[1]->IsFunction()); CHECK(args[2]->IsFunction()); - CHECK(args[3]->IsFunction()); - - // Attach Fields enum from Environment::AsyncHooks. - // Flags attached to this object are: - // - kCallInitHook (0): Tells the AsyncWrap constructor whether it should - // make a call to the init JS callback. This is disabled by default, so - // even after setting the callbacks the flag will have to be set to - // non-zero to have those callbacks called. This only affects the init - // callback. If the init callback was called, then the pre/post callbacks - // will automatically be called. - Local async_hooks_obj = args[0].As(); - Environment::AsyncHooks* async_hooks = env->async_hooks(); - async_hooks_obj->SetIndexedPropertiesToExternalArrayData( - async_hooks->fields(), - kExternalUint32Array, - async_hooks->fields_count()); - - env->set_async_hooks_init_function(args[1].As()); - env->set_async_hooks_pre_function(args[2].As()); - env->set_async_hooks_post_function(args[3].As()); + + env->set_async_hooks_init_function(args[0].As()); + env->set_async_hooks_pre_function(args[1].As()); + env->set_async_hooks_post_function(args[2].As()); env->set_using_asyncwrap(true); } @@ -62,6 +58,8 @@ static void Initialize(Handle target, HandleScope scope(isolate); NODE_SET_METHOD(target, "setupHooks", SetupHooks); + env->SetMethod(target, "disable", DisableHooksJS); + env->SetMethod(target, "enable", EnableHooksJS); Local async_providers = Object::New(isolate); #define V(PROVIDER) \ From eed38b373ef94e1563323c6165ea81f6e95ac91b Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 4 May 2015 16:11:58 -0600 Subject: [PATCH 4/4] async-wrap: remove before/after calls in init It doesn't make sense to call before/after callbacks in init to the parent because they'll be made anyway from MakeCallback. If information does need to be propagated then it should be done automatically. Will deal with this if the issue arrises in the future. --- src/async-wrap-inl.h | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index 46d0e19e536eed..bad634ddaf14b9 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -30,24 +30,9 @@ inline AsyncWrap::AsyncWrap(Environment* env, if (!env->async_wrap_callbacks_enabled() && !parent->has_async_queue()) return; - // TODO(trevnorris): Until it's verified all passed object's are not weak, - // add a HandleScope to make sure there's no leak. v8::HandleScope scope(env->isolate()); - - v8::Local parent_obj; - v8::TryCatch try_catch; - // If a parent value was sent then call its pre/post functions to let it know - // a conceptual "child" is being instantiated (e.g. that a server has - // received a connection). - if (parent != nullptr) { - parent_obj = parent->object(); - env->async_hooks_pre_function()->Call(parent_obj, 0, nullptr); - if (try_catch.HasCaught()) - FatalError("node::AsyncWrap::AsyncWrap", "parent pre hook threw"); - } - v8::Local n = v8::Int32::New(env->isolate(), provider); env->async_hooks_init_function()->Call(object, 1, &n); @@ -55,12 +40,6 @@ inline AsyncWrap::AsyncWrap(Environment* env, FatalError("node::AsyncWrap::AsyncWrap", "init hook threw"); bits_ |= 1; // has_async_queue() is true now. - - if (parent != nullptr) { - env->async_hooks_post_function()->Call(parent_obj, 0, nullptr); - if (try_catch.HasCaught()) - FatalError("node::AsyncWrap::AsyncWrap", "parent post hook threw"); - } }