From e9192249c8c6b05c7cc04e8b51a7e37235f31415 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 5 Jan 2016 15:33:21 -0700 Subject: [PATCH] src: add AsyncCallbackScope Add a scope that will allow MakeCallback to know whether or not it's currently running. This will prevent nextTickQueue and the MicrotaskQueue from being processed recursively. It is also required to wrap the bootloading stage since it doesn't run within a MakeCallback. Ref: https://github.com/nodejs/node-v0.x-archive/issues/9245 PR-URL: https://github.com/nodejs/node/pull/4507 Reviewed-By: Fedor Indutny --- src/async-wrap.cc | 8 +++----- src/env-inl.h | 15 +++++++++++++++ src/env.cc | 8 ++------ src/env.h | 16 +++++++++++++++- src/node.cc | 9 +++++++-- src/node_http_parser.cc | 4 +++- src/node_internals.h | 2 -- 7 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 29ea139f5f..01dcaf277c 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -184,6 +184,8 @@ Local AsyncWrap::MakeCallback(const Local cb, Local domain; 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(); @@ -236,7 +238,7 @@ Local AsyncWrap::MakeCallback(const Local cb, Environment::TickInfo* tick_info = env()->tick_info(); - if (tick_info->in_tick()) { + if (callback_scope.in_makecallback()) { return ret; } @@ -249,12 +251,8 @@ Local AsyncWrap::MakeCallback(const Local cb, return ret; } - tick_info->set_in_tick(true); - env()->tick_callback_function()->Call(process, 0, nullptr); - tick_info->set_in_tick(false); - if (try_catch.HasCaught()) { tick_info->set_last_threw(true); return Undefined(env()->isolate()); diff --git a/src/env-inl.h b/src/env-inl.h index 46272585d5..c09fb87897 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -101,6 +101,20 @@ inline void Environment::AsyncHooks::set_enable_callbacks(uint32_t flag) { fields_[kEnableCallbacks] = flag; } +inline Environment::AsyncCallbackScope::AsyncCallbackScope(Environment* env) + : env_(env) { + env_->makecallback_cntr_++; +} + +inline Environment::AsyncCallbackScope::~AsyncCallbackScope() { + env_->makecallback_cntr_--; + CHECK_GE(env_->makecallback_cntr_, 0); +} + +inline bool Environment::AsyncCallbackScope::in_makecallback() { + return env_->makecallback_cntr_ > 1; +} + inline Environment::DomainFlag::DomainFlag() { for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0; } @@ -223,6 +237,7 @@ inline Environment::Environment(v8::Local context, using_domains_(false), printed_error_(false), trace_sync_io_(false), + makecallback_cntr_(0), async_wrap_uid_(0), debugger_agent_(this), http_parser_buffer_(nullptr), diff --git a/src/env.cc b/src/env.cc index fa8cc0d1ad..b852a630d5 100644 --- a/src/env.cc +++ b/src/env.cc @@ -64,10 +64,10 @@ void Environment::PrintSyncTrace() const { } -bool Environment::KickNextTick() { +bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) { TickInfo* info = tick_info(); - if (info->in_tick()) { + if (scope->in_makecallback()) { return true; } @@ -80,15 +80,11 @@ bool Environment::KickNextTick() { return true; } - info->set_in_tick(true); - // process nextTicks after call TryCatch try_catch; try_catch.SetVerbose(true); tick_callback_function()->Call(process_object(), 0, nullptr); - info->set_in_tick(false); - if (try_catch.HasCaught()) { info->set_last_threw(true); return false; diff --git a/src/env.h b/src/env.h index e160e62310..e78cd9d94a 100644 --- a/src/env.h +++ b/src/env.h @@ -314,6 +314,19 @@ class Environment { DISALLOW_COPY_AND_ASSIGN(AsyncHooks); }; + class AsyncCallbackScope { + public: + explicit AsyncCallbackScope(Environment* env); + ~AsyncCallbackScope(); + + inline bool in_makecallback(); + + private: + Environment* env_; + + DISALLOW_COPY_AND_ASSIGN(AsyncCallbackScope); + }; + class DomainFlag { public: inline uint32_t* fields(); @@ -466,7 +479,7 @@ class Environment { inline int64_t get_async_wrap_uid(); - bool KickNextTick(); + bool KickNextTick(AsyncCallbackScope* scope); inline uint32_t* heap_statistics_buffer() const; inline void set_heap_statistics_buffer(uint32_t* pointer); @@ -569,6 +582,7 @@ class Environment { bool using_domains_; bool printed_error_; bool trace_sync_io_; + size_t makecallback_cntr_; int64_t async_wrap_uid_; debugger::Agent debugger_agent_; diff --git a/src/node.cc b/src/node.cc index c0049a702e..d9d27f7a7a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1140,6 +1140,8 @@ Local MakeCallback(Environment* env, bool ran_init_callback = false; bool has_domain = false; + Environment::AsyncCallbackScope callback_scope(env); + // TODO(trevnorris): Adding "_asyncQueue" to the "this" in the init callback // is a horrible way to detect usage. Rethink how detection should happen. if (recv->IsObject()) { @@ -1200,7 +1202,7 @@ Local MakeCallback(Environment* env, } } - if (!env->KickNextTick()) + if (!env->KickNextTick(&callback_scope)) return Undefined(env->isolate()); return ret; @@ -4151,7 +4153,10 @@ static void StartNodeInstance(void* arg) { if (instance_data->use_debug_agent()) StartDebug(env, debug_wait_connect); - LoadEnvironment(env); + { + Environment::AsyncCallbackScope callback_scope(env); + LoadEnvironment(env); + } env->set_trace_sync_io(trace_sync_io); diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 28322f95c4..12c6fcff86 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -578,6 +578,8 @@ class Parser : public BaseObject { if (!cb->IsFunction()) return; + Environment::AsyncCallbackScope callback_scope(parser->env()); + // Hooks for GetCurrentBuffer parser->current_buffer_len_ = nread; parser->current_buffer_data_ = buf->base; @@ -587,7 +589,7 @@ class Parser : public BaseObject { parser->current_buffer_len_ = 0; parser->current_buffer_data_ = nullptr; - parser->env()->KickNextTick(); + parser->env()->KickNextTick(&callback_scope); } diff --git a/src/node_internals.h b/src/node_internals.h index ea83d4d9fc..6a91876494 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -69,8 +69,6 @@ v8::Local MakeCallback(Environment* env, int argc = 0, v8::Local* argv = nullptr); -bool KickNextTick(); - // 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.