diff --git a/src/async-wrap.cc b/src/async-wrap.cc index c9f5caad1e4ea8..a7a9822238329a 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(); @@ -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(); @@ -194,52 +196,45 @@ 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()) { - try_catch.SetVerbose(false); - 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"); - 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()) + if (post_fn->Call(context, 0, nullptr).IsEmpty()) FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); - try_catch.SetVerbose(true); + } + + if (ret.IsEmpty()) { + return Undefined(env()->isolate()); } 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"); + } } } Environment::TickInfo* tick_info = env()->tick_info(); - if (tick_info->in_tick()) { + if (callback_scope.in_makecallback()) { return ret; } @@ -252,14 +247,7 @@ 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); + if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { return Undefined(env()->isolate()); } diff --git a/src/env-inl.h b/src/env-inl.h index 46272585d5434d..e1856d81b0060a 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; } @@ -117,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() { for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0; } @@ -130,34 +144,18 @@ 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]; } -inline bool Environment::TickInfo::last_threw() const { - return last_threw_; -} - 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; } -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; @@ -223,6 +221,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..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_) @@ -64,10 +65,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,21 +81,10 @@ bool Environment::KickNextTick() { return true; } - info->set_in_tick(true); + Local ret = + tick_callback_function()->Call(process_object(), 0, nullptr); - // 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; - } - - return true; + return !ret.IsEmpty(); } } // namespace node diff --git a/src/env.h b/src/env.h index e160e62310e8ec..80f43036f2dd51 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(); @@ -338,13 +351,9 @@ class Environment { public: 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. @@ -357,8 +366,6 @@ class Environment { }; uint32_t fields_[kFieldsCount]; - bool in_tick_; - bool last_threw_; DISALLOW_COPY_AND_ASSIGN(TickInfo); }; @@ -466,7 +473,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 +576,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 4609422497fe68..05984745de080f 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()); @@ -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()) { @@ -1160,52 +1162,46 @@ Local MakeCallback(Environment* env, } } - TryCatch try_catch; - 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()) { - try_catch.SetVerbose(false); - pre_fn->Call(object, 0, nullptr); - if (try_catch.HasCaught()) + if (pre_fn->Call(object, 0, nullptr).IsEmpty()) 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()) + if (post_fn->Call(object, 0, nullptr).IsEmpty()) FatalError("node::MakeCallback", "post hook threw"); - try_catch.SetVerbose(true); + } + + if (ret.IsEmpty()) { + return Undefined(env->isolate()); } 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 (try_catch.HasCaught()) { + if (!env->KickNextTick(&callback_scope)) { return Undefined(env->isolate()); } - if (!env->KickNextTick()) - return Undefined(env->isolate()); - return ret; } @@ -4154,7 +4150,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. 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'); + })); +}