From 575b84ec41b94d4a565b981c146d88ce0637fff2 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 11 Feb 2016 13:09:52 -0700 Subject: [PATCH 1/6] src: fix MakeCallback error handling Implementations of error handling between node::MakeCallback() and AsyncWrap::MakeCallback() do not return at the same point. Make both executions work the same by moving the early return if there's a caught exception just after the AsyncWrap post callback. Since the domain's call stack is cleared on a caught exception there is no reason to call its exit() callback. Remove the SetVerbose() statement in the AsyncWrap pre/post callback calls since it does not affect the callback call. PR-URL: https://github.com/nodejs/node/pull/4507 Reviewed-By: Fedor Indutny --- src/async-wrap.cc | 13 +++++-------- src/node.cc | 13 +++++-------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index c9f5caad1e4ea8..29ea139f5f91c0 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -207,25 +207,22 @@ Local AsyncWrap::MakeCallback(const Local cb, } if (ran_init_callback() && !pre_fn.IsEmpty()) { - try_catch.SetVerbose(false); pre_fn->Call(context, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::AsyncWrap::MakeCallback", "pre hook threw"); - try_catch.SetVerbose(true); } Local ret = cb->Call(context, argc, argv); - if (try_catch.HasCaught()) { - return Undefined(env()->isolate()); - } - if (ran_init_callback() && !post_fn.IsEmpty()) { - try_catch.SetVerbose(false); post_fn->Call(context, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); - try_catch.SetVerbose(true); + } + + // If the return value is empty then the callback threw. + if (ret.IsEmpty()) { + return Undefined(env()->isolate()); } if (has_domain) { diff --git a/src/node.cc b/src/node.cc index 4609422497fe68..c0049a702e77bf 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1173,21 +1173,22 @@ Local MakeCallback(Environment* env, } if (ran_init_callback && !pre_fn.IsEmpty()) { - try_catch.SetVerbose(false); pre_fn->Call(object, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::MakeCallback", "pre hook threw"); - try_catch.SetVerbose(true); } Local ret = callback->Call(recv, argc, argv); if (ran_init_callback && !post_fn.IsEmpty()) { - try_catch.SetVerbose(false); post_fn->Call(object, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::MakeCallback", "post hook threw"); - try_catch.SetVerbose(true); + } + + // If the return value is empty then the callback threw. + if (ret.IsEmpty()) { + return Undefined(env->isolate()); } if (has_domain) { @@ -1199,10 +1200,6 @@ Local MakeCallback(Environment* env, } } - if (try_catch.HasCaught()) { - return Undefined(env->isolate()); - } - if (!env->KickNextTick()) return Undefined(env->isolate()); From e9192249c8c6b05c7cc04e8b51a7e37235f31415 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 5 Jan 2016 15:33:21 -0700 Subject: [PATCH 2/6] 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 29ea139f5f91c0..01dcaf277c1840 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 46272585d5434d..c09fb87897c070 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 fa8cc0d1addfd7..b852a630d5ba99 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 e160e62310e8ec..e78cd9d94a5d76 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 c0049a702e77bf..d9d27f7a7a6a44 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 28322f95c40939..12c6fcff86c145 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 ea83d4d9fc1238..6a918764948793 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. From f86a3a2fb567c226d0849c9c8a69118f97578528 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 11 Feb 2016 13:10:47 -0700 Subject: [PATCH 3/6] src: remove unused of TickInfo::last_threw() Environment::TickInfo::last_threw() is no longer in use. Also pass Isolate to few methods and fix whitespace alignment. PR-URL: https://github.com/nodejs/node/pull/4507 Reviewed-By: Fedor Indutny --- src/async-wrap.cc | 5 ++--- src/env-inl.h | 10 +--------- src/env.cc | 3 +-- src/env.h | 3 --- src/node.cc | 10 +++++----- 5 files changed, 9 insertions(+), 22 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 01dcaf277c1840..789e357c732773 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -173,8 +173,8 @@ void LoadAsyncWrapperInfo(Environment* env) { Local AsyncWrap::MakeCallback(const Local cb, - int argc, - Local* argv) { + int argc, + Local* argv) { CHECK(env()->context() == env()->isolate()->GetCurrentContext()); Local pre_fn = env()->async_hooks_pre_function(); @@ -254,7 +254,6 @@ Local AsyncWrap::MakeCallback(const Local cb, env()->tick_callback_function()->Call(process, 0, nullptr); 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 c09fb87897c070..423002debb192c 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -131,7 +131,7 @@ inline uint32_t Environment::DomainFlag::count() const { return fields_[kCount]; } -inline Environment::TickInfo::TickInfo() : in_tick_(false), last_threw_(false) { +inline Environment::TickInfo::TickInfo() : in_tick_(false) { for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0; } @@ -152,10 +152,6 @@ inline uint32_t Environment::TickInfo::index() const { return fields_[kIndex]; } -inline bool Environment::TickInfo::last_threw() const { - return last_threw_; -} - inline uint32_t Environment::TickInfo::length() const { return fields_[kLength]; } @@ -168,10 +164,6 @@ inline void Environment::TickInfo::set_index(uint32_t value) { fields_[kIndex] = value; } -inline void Environment::TickInfo::set_last_threw(bool value) { - last_threw_ = value; -} - inline Environment::ArrayBufferAllocatorInfo::ArrayBufferAllocatorInfo() { for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0; diff --git a/src/env.cc b/src/env.cc index b852a630d5ba99..b79fe965aabbae 100644 --- a/src/env.cc +++ b/src/env.cc @@ -81,12 +81,11 @@ bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) { } // process nextTicks after call - TryCatch try_catch; + TryCatch try_catch(isolate()); try_catch.SetVerbose(true); tick_callback_function()->Call(process_object(), 0, nullptr); if (try_catch.HasCaught()) { - info->set_last_threw(true); return false; } diff --git a/src/env.h b/src/env.h index e78cd9d94a5d76..ec106933f02206 100644 --- a/src/env.h +++ b/src/env.h @@ -352,12 +352,10 @@ class Environment { inline uint32_t* fields(); inline int fields_count() const; inline bool in_tick() const; - inline bool last_threw() const; inline uint32_t index() const; inline uint32_t length() const; inline void set_in_tick(bool value); inline void set_index(uint32_t value); - inline void set_last_threw(bool value); private: friend class Environment; // So we can call the constructor. @@ -371,7 +369,6 @@ class Environment { uint32_t fields_[kFieldsCount]; bool in_tick_; - bool last_threw_; DISALLOW_COPY_AND_ASSIGN(TickInfo); }; diff --git a/src/node.cc b/src/node.cc index d9d27f7a7a6a44..cae4da12c99843 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1127,10 +1127,10 @@ void SetupPromises(const FunctionCallbackInfo& args) { Local MakeCallback(Environment* env, - Local recv, - const Local callback, - int argc, - Local argv[]) { + Local recv, + const Local callback, + int argc, + Local argv[]) { // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); @@ -1162,7 +1162,7 @@ Local MakeCallback(Environment* env, } } - TryCatch try_catch; + TryCatch try_catch(env->isolate()); try_catch.SetVerbose(true); if (has_domain) { From 5e30478ede524113b012d9fe94597f64188d386f Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 10 Feb 2016 12:48:44 -0700 Subject: [PATCH 4/6] src: remove unused TickInfo::in_tick() PR-URL: https://github.com/nodejs/node/pull/4507 Reviewed-By: Fedor Indutny --- src/env-inl.h | 10 +--------- src/env.h | 3 --- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 423002debb192c..e1856d81b0060a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -131,7 +131,7 @@ inline uint32_t Environment::DomainFlag::count() const { return fields_[kCount]; } -inline Environment::TickInfo::TickInfo() : in_tick_(false) { +inline Environment::TickInfo::TickInfo() { for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0; } @@ -144,10 +144,6 @@ inline int Environment::TickInfo::fields_count() const { return kFieldsCount; } -inline bool Environment::TickInfo::in_tick() const { - return in_tick_; -} - inline uint32_t Environment::TickInfo::index() const { return fields_[kIndex]; } @@ -156,10 +152,6 @@ inline uint32_t Environment::TickInfo::length() const { return fields_[kLength]; } -inline void Environment::TickInfo::set_in_tick(bool value) { - in_tick_ = value; -} - inline void Environment::TickInfo::set_index(uint32_t value) { fields_[kIndex] = value; } diff --git a/src/env.h b/src/env.h index ec106933f02206..80f43036f2dd51 100644 --- a/src/env.h +++ b/src/env.h @@ -351,10 +351,8 @@ class Environment { public: inline uint32_t* fields(); inline int fields_count() const; - inline bool in_tick() const; inline uint32_t index() const; inline uint32_t length() const; - inline void set_in_tick(bool value); inline void set_index(uint32_t value); private: @@ -368,7 +366,6 @@ class Environment { }; uint32_t fields_[kFieldsCount]; - bool in_tick_; DISALLOW_COPY_AND_ASSIGN(TickInfo); }; From 95afe28fc5312c97e61cf998d0a9d065633e2147 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 11 Feb 2016 13:57:26 -0700 Subject: [PATCH 5/6] src: remove TryCatch in MakeCallback After attempting to use ReThrow() and Reset() there were cases where firing the domain's error handlers was not happening. Or in some cases reentering MakeCallback would still cause the domain enter callback to abort (because the error had not been Reset yet). In order for the script to properly stop execution when a subsequent call to MakeCallback throws it must not be located within a TryCatch. PR-URL: https://github.com/nodejs/node/pull/4507 Reviewed-By: Fedor Indutny --- src/async-wrap.cc | 28 +++++++++++----------------- src/env.cc | 13 ++++--------- src/node.cc | 27 ++++++++++++--------------- 3 files changed, 27 insertions(+), 41 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 789e357c732773..a7a9822238329a 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -196,33 +196,28 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - TryCatch try_catch(env()->isolate()); - try_catch.SetVerbose(true); - if (has_domain) { Local enter_v = domain->Get(env()->enter_string()); if (enter_v->IsFunction()) { - enter_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env()->isolate()); + if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain enter callback threw, please report this"); + } } } if (ran_init_callback() && !pre_fn.IsEmpty()) { - pre_fn->Call(context, 0, nullptr); - if (try_catch.HasCaught()) + if (pre_fn->Call(context, 0, nullptr).IsEmpty()) FatalError("node::AsyncWrap::MakeCallback", "pre hook threw"); } Local ret = cb->Call(context, argc, argv); if (ran_init_callback() && !post_fn.IsEmpty()) { - post_fn->Call(context, 0, nullptr); - if (try_catch.HasCaught()) + if (post_fn->Call(context, 0, nullptr).IsEmpty()) FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); } - // If the return value is empty then the callback threw. if (ret.IsEmpty()) { return Undefined(env()->isolate()); } @@ -230,9 +225,10 @@ Local AsyncWrap::MakeCallback(const Local cb, if (has_domain) { Local exit_v = domain->Get(env()->exit_string()); if (exit_v->IsFunction()) { - exit_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env()->isolate()); + if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain exit callback threw, please report this"); + } } } @@ -251,9 +247,7 @@ Local AsyncWrap::MakeCallback(const Local cb, return ret; } - env()->tick_callback_function()->Call(process, 0, nullptr); - - if (try_catch.HasCaught()) { + if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { return Undefined(env()->isolate()); } diff --git a/src/env.cc b/src/env.cc index b79fe965aabbae..75a628face1fe8 100644 --- a/src/env.cc +++ b/src/env.cc @@ -18,6 +18,7 @@ using v8::Message; using v8::StackFrame; using v8::StackTrace; using v8::TryCatch; +using v8::Value; void Environment::PrintSyncTrace() const { if (!trace_sync_io_) @@ -80,16 +81,10 @@ bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) { return true; } - // process nextTicks after call - TryCatch try_catch(isolate()); - try_catch.SetVerbose(true); - tick_callback_function()->Call(process_object(), 0, nullptr); + Local ret = + tick_callback_function()->Call(process_object(), 0, nullptr); - if (try_catch.HasCaught()) { - return false; - } - - return true; + return !ret.IsEmpty(); } } // namespace node diff --git a/src/node.cc b/src/node.cc index cae4da12c99843..05984745de080f 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1162,33 +1162,28 @@ Local MakeCallback(Environment* env, } } - TryCatch try_catch(env->isolate()); - try_catch.SetVerbose(true); - if (has_domain) { Local enter_v = domain->Get(env->enter_string()); if (enter_v->IsFunction()) { - enter_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env->isolate()); + if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::MakeCallback", + "domain enter callback threw, please report this"); + } } } if (ran_init_callback && !pre_fn.IsEmpty()) { - pre_fn->Call(object, 0, nullptr); - if (try_catch.HasCaught()) + if (pre_fn->Call(object, 0, nullptr).IsEmpty()) FatalError("node::MakeCallback", "pre hook threw"); } Local ret = callback->Call(recv, argc, argv); if (ran_init_callback && !post_fn.IsEmpty()) { - post_fn->Call(object, 0, nullptr); - if (try_catch.HasCaught()) + if (post_fn->Call(object, 0, nullptr).IsEmpty()) FatalError("node::MakeCallback", "post hook threw"); } - // If the return value is empty then the callback threw. if (ret.IsEmpty()) { return Undefined(env->isolate()); } @@ -1196,14 +1191,16 @@ Local MakeCallback(Environment* env, if (has_domain) { Local exit_v = domain->Get(env->exit_string()); if (exit_v->IsFunction()) { - exit_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env->isolate()); + if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::MakeCallback", + "domain exit callback threw, please report this"); + } } } - if (!env->KickNextTick(&callback_scope)) + if (!env->KickNextTick(&callback_scope)) { return Undefined(env->isolate()); + } return ret; } From ab6fe5998a625af68b58ccc63b230ef37c054af7 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 6 Jan 2016 10:39:40 -0700 Subject: [PATCH 6/6] test: add addons test for MakeCallback Make sure that calling MakeCallback multiple times within the same stack does not allow the nextTickQueue or MicrotaskQueue to be processed in any more than the first MakeCallback call. Check that domains enter/exit poperly with multiple MakeCallback calls and that errors are handled as expected PR-URL: https://github.com/nodejs/node/pull/4507 Reviewed-By: Fedor Indutny --- test/addons/make-callback-recurse/binding.cc | 31 ++++ test/addons/make-callback-recurse/binding.gyp | 8 + test/addons/make-callback-recurse/test.js | 170 ++++++++++++++++++ 3 files changed, 209 insertions(+) create mode 100644 test/addons/make-callback-recurse/binding.cc create mode 100644 test/addons/make-callback-recurse/binding.gyp create mode 100644 test/addons/make-callback-recurse/test.js diff --git a/test/addons/make-callback-recurse/binding.cc b/test/addons/make-callback-recurse/binding.cc new file mode 100644 index 00000000000000..1c575910ef66f5 --- /dev/null +++ b/test/addons/make-callback-recurse/binding.cc @@ -0,0 +1,31 @@ +#include "node.h" +#include "v8.h" + +#include "../../../src/util.h" + +using v8::Function; +using v8::FunctionCallbackInfo; +using v8::Isolate; +using v8::Local; +using v8::Object; +using v8::Value; + +namespace { + +void MakeCallback(const FunctionCallbackInfo& args) { + CHECK(args[0]->IsObject()); + CHECK(args[1]->IsFunction()); + Isolate* isolate = args.GetIsolate(); + Local recv = args[0].As(); + Local method = args[1].As(); + + node::MakeCallback(isolate, recv, method, 0, nullptr); +} + +void Initialize(Local target) { + NODE_SET_METHOD(target, "makeCallback", MakeCallback); +} + +} // namespace anonymous + +NODE_MODULE(binding, Initialize) diff --git a/test/addons/make-callback-recurse/binding.gyp b/test/addons/make-callback-recurse/binding.gyp new file mode 100644 index 00000000000000..3bfb84493f3e87 --- /dev/null +++ b/test/addons/make-callback-recurse/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons/make-callback-recurse/test.js b/test/addons/make-callback-recurse/test.js new file mode 100644 index 00000000000000..f924ac61916ba5 --- /dev/null +++ b/test/addons/make-callback-recurse/test.js @@ -0,0 +1,170 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const domain = require('domain'); +const binding = require('./build/Release/binding'); +const makeCallback = binding.makeCallback; + +// Make sure this is run in the future. +const mustCallCheckDomains = common.mustCall(checkDomains); + + +// Make sure that using MakeCallback allows the error to propagate. +assert.throws(function() { + makeCallback({}, function() { + throw new Error('hi from domain error'); + }); +}); + + +// Check the execution order of the nextTickQueue and MicrotaskQueue in +// relation to running multiple MakeCallback's from bootstrap, +// node::MakeCallback() and node::AsyncWrap::MakeCallback(). +// TODO(trevnorris): Is there a way to verify this is being run during +// bootstrap? +(function verifyExecutionOrder(arg) { + const results_arr = []; + + // Processing of the MicrotaskQueue is manually handled by node. They are not + // processed until after the nextTickQueue has been processed. + Promise.resolve(1).then(common.mustCall(function() { + results_arr.push(7); + })); + + // The nextTick should run after all immediately invoked calls. + process.nextTick(common.mustCall(function() { + results_arr.push(3); + + // Run same test again but while processing the nextTickQueue to make sure + // the following MakeCallback call breaks in the middle of processing the + // queue and allows the script to run normally. + process.nextTick(common.mustCall(function() { + results_arr.push(6); + })); + + makeCallback({}, common.mustCall(function() { + results_arr.push(4); + })); + + results_arr.push(5); + })); + + results_arr.push(0); + + // MakeCallback is calling the function immediately, but should then detect + // that a script is already in the middle of execution and return before + // either the nextTickQueue or MicrotaskQueue are processed. + makeCallback({}, common.mustCall(function() { + results_arr.push(1); + })); + + // This should run before either the nextTickQueue or MicrotaskQueue are + // processed. Previously MakeCallback would not detect this circumstance + // and process them immediately. + results_arr.push(2); + + setImmediate(common.mustCall(function() { + for (var i = 0; i < results_arr.length; i++) { + assert.equal(results_arr[i], + i, + `verifyExecutionOrder(${arg}) results: ${results_arr}`); + } + if (arg === 1) { + // The tests are first run on bootstrap during LoadEnvironment() in + // src/node.cc. Now run the tests through node::MakeCallback(). + setImmediate(function() { + makeCallback({}, common.mustCall(function() { + verifyExecutionOrder(2); + })); + }); + } else if (arg === 2) { + // setTimeout runs via the TimerWrap, which runs through + // AsyncWrap::MakeCallback(). Make sure there are no conflicts using + // node::MakeCallback() within it. + setTimeout(common.mustCall(function() { + verifyExecutionOrder(3); + }), 10); + } else if (arg === 3) { + mustCallCheckDomains(); + } else { + throw new Error('UNREACHABLE'); + } + })); +}(1)); + + +function checkDomains() { + // Check that domains are properly entered/exited when called in multiple + // levels from both node::MakeCallback() and AsyncWrap::MakeCallback + setImmediate(common.mustCall(function() { + const d1 = domain.create(); + const d2 = domain.create(); + const d3 = domain.create(); + + makeCallback({ domain: d1 }, common.mustCall(function() { + assert.equal(d1, process.domain); + makeCallback({ domain: d2 }, common.mustCall(function() { + assert.equal(d2, process.domain); + makeCallback({ domain: d3 }, common.mustCall(function() { + assert.equal(d3, process.domain); + })); + assert.equal(d2, process.domain); + })); + assert.equal(d1, process.domain); + })); + })); + + setTimeout(common.mustCall(function() { + const d1 = domain.create(); + const d2 = domain.create(); + const d3 = domain.create(); + + makeCallback({ domain: d1 }, common.mustCall(function() { + assert.equal(d1, process.domain); + makeCallback({ domain: d2 }, common.mustCall(function() { + assert.equal(d2, process.domain); + makeCallback({ domain: d3 }, common.mustCall(function() { + assert.equal(d3, process.domain); + })); + assert.equal(d2, process.domain); + })); + assert.equal(d1, process.domain); + })); + }), 1); + + // Make sure nextTick, setImmediate and setTimeout can all recover properly + // after a thrown makeCallback call. + process.nextTick(common.mustCall(function() { + const d = domain.create(); + d.on('error', common.mustCall(function(e) { + assert.equal(e.message, 'throw from domain 3'); + })); + makeCallback({ domain: d }, function() { + throw new Error('throw from domain 3'); + }); + throw new Error('UNREACHABLE'); + })); + + setImmediate(common.mustCall(function() { + const d = domain.create(); + d.on('error', common.mustCall(function(e) { + assert.equal(e.message, 'throw from domain 2'); + })); + makeCallback({ domain: d }, function() { + throw new Error('throw from domain 2'); + }); + throw new Error('UNREACHABLE'); + })); + + setTimeout(common.mustCall(function() { + const d = domain.create(); + d.on('error', common.mustCall(function(e) { + assert.equal(e.message, 'throw from domain 1'); + })); + makeCallback({ domain: d }, function() { + throw new Error('throw from domain 1'); + }); + throw new Error('UNREACHABLE'); + })); +}