diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 391a684759..fe53d27994 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -657,73 +657,8 @@ void AsyncWrap::EmitAsyncInit(Environment* env, MaybeLocal AsyncWrap::MakeCallback(const Local cb, int argc, Local* argv) { - CHECK(env()->context() == env()->isolate()->GetCurrentContext()); - - Environment::AsyncCallbackScope callback_scope(env()); - - Environment::AsyncHooks::ExecScope exec_scope(env(), - get_id(), - get_trigger_id()); - - // Return v8::Undefined() because returning an empty handle will cause - // ToLocalChecked() to abort. - if (env()->using_domains() && DomainEnter(env(), object())) { - return Undefined(env()->isolate()); - } - - // No need to check a return value because the application will exit if an - // exception occurs. - AsyncWrap::EmitBefore(env(), get_id()); - - MaybeLocal ret = cb->Call(env()->context(), object(), argc, argv); - - if (ret.IsEmpty()) { - return ret; - } - - AsyncWrap::EmitAfter(env(), get_id()); - - // Return v8::Undefined() because returning an empty handle will cause - // ToLocalChecked() to abort. - if (env()->using_domains() && DomainExit(env(), object())) { - return Undefined(env()->isolate()); - } - - exec_scope.Dispose(); - - if (callback_scope.in_makecallback()) { - return ret; - } - - Environment::TickInfo* tick_info = env()->tick_info(); - - if (tick_info->length() == 0) { - env()->isolate()->RunMicrotasks(); - } - - // Make sure the stack unwound properly. If there are nested MakeCallback's - // then it should return early and not reach this code. - CHECK_EQ(env()->current_async_id(), 0); - CHECK_EQ(env()->trigger_id(), 0); - - Local process = env()->process_object(); - - if (tick_info->length() == 0) { - tick_info->set_index(0); - return ret; - } - - MaybeLocal rcheck = - env()->tick_callback_function()->Call(env()->context(), - process, - 0, - nullptr); - - // Make sure the stack unwound properly. - CHECK_EQ(env()->current_async_id(), 0); - CHECK_EQ(env()->trigger_id(), 0); - - return rcheck.IsEmpty() ? MaybeLocal() : ret; + async_context context { get_id(), get_trigger_id() }; + return InternalMakeCallback(env(), object(), cb, argc, argv, context); } diff --git a/src/node.cc b/src/node.cc index 30cbb11efa..cdbb7edb9e 100644 --- a/src/node.cc +++ b/src/node.cc @@ -161,6 +161,7 @@ using v8::SealHandleScope; using v8::String; using v8::TryCatch; using v8::Uint32Array; +using v8::Undefined; using v8::V8; using v8::Value; @@ -1311,85 +1312,153 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { env->AddPromiseHook(fn, arg); } +class InternalCallbackScope { + public: + InternalCallbackScope(Environment* env, + Local object, + const async_context& asyncContext); + ~InternalCallbackScope(); + void Close(); + + inline bool Failed() const { return failed_; } + inline void MarkAsFailed() { failed_ = true; } + inline bool IsInnerMakeCallback() const { + return callback_scope_.in_makecallback(); + } + + private: + Environment* env_; + async_context async_context_; + v8::Local object_; + Environment::AsyncCallbackScope callback_scope_; + bool failed_ = false; + bool pushed_ids_ = false; + bool closed_ = false; +}; -MaybeLocal MakeCallback(Environment* env, - Local recv, - const Local callback, - int argc, - Local argv[], - async_context asyncContext) { - // If you hit this assertion, you forgot to enter the v8::Context first. - CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); +CallbackScope::CallbackScope(Isolate* isolate, + Local object, + async_context asyncContext) + : private_(new InternalCallbackScope(Environment::GetCurrent(isolate), + object, + asyncContext)), + try_catch_(isolate) { + try_catch_.SetVerbose(true); +} - Local object; +CallbackScope::~CallbackScope() { + if (try_catch_.HasCaught()) + private_->MarkAsFailed(); + delete private_; +} - Environment::AsyncCallbackScope callback_scope(env); - bool disposed_domain = false; +InternalCallbackScope::InternalCallbackScope(Environment* env, + Local object, + const async_context& asyncContext) + : env_(env), + async_context_(asyncContext), + object_(object), + callback_scope_(env) { + CHECK(!object.IsEmpty()); - if (recv->IsObject()) { - object = recv.As(); - } + // If you hit this assertion, you forgot to enter the v8::Context first. + CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); if (env->using_domains()) { - CHECK(recv->IsObject()); - disposed_domain = DomainEnter(env, object); - if (disposed_domain) return Undefined(env->isolate()); + failed_ = DomainEnter(env, object_); + if (failed_) + return; } - MaybeLocal ret; + if (asyncContext.async_id != 0) { + // No need to check a return value because the application will exit if + // an exception occurs. + AsyncWrap::EmitBefore(env, asyncContext.async_id); + } - { - AsyncHooks::ExecScope exec_scope(env, asyncContext.async_id, - asyncContext.trigger_async_id); + env->async_hooks()->push_ids(async_context_.async_id, + async_context_.trigger_async_id); + pushed_ids_ = true; +} - if (asyncContext.async_id != 0) { - // No need to check a return value because the application will exit if - // an exception occurs. - AsyncWrap::EmitBefore(env, asyncContext.async_id); - } +InternalCallbackScope::~InternalCallbackScope() { + Close(); +} - ret = callback->Call(env->context(), recv, argc, argv); +void InternalCallbackScope::Close() { + if (closed_) return; + closed_ = true; - if (ret.IsEmpty()) { - // NOTE: For backwards compatibility with public API we return Undefined() - // if the top level call threw. - return callback_scope.in_makecallback() ? - ret : Undefined(env->isolate()); - } + if (pushed_ids_) + env_->async_hooks()->pop_ids(async_context_.async_id); - if (asyncContext.async_id != 0) { - AsyncWrap::EmitAfter(env, asyncContext.async_id); - } + if (failed_) return; + + if (async_context_.async_id != 0) { + AsyncWrap::EmitAfter(env_, async_context_.async_id); } - if (env->using_domains()) { - disposed_domain = DomainExit(env, object); - if (disposed_domain) return Undefined(env->isolate()); + if (env_->using_domains()) { + failed_ = DomainExit(env_, object_); + if (failed_) return; } - if (callback_scope.in_makecallback()) { - return ret; + if (IsInnerMakeCallback()) { + return; } - Environment::TickInfo* tick_info = env->tick_info(); + Environment::TickInfo* tick_info = env_->tick_info(); if (tick_info->length() == 0) { - env->isolate()->RunMicrotasks(); + env_->isolate()->RunMicrotasks(); } // Make sure the stack unwound properly. If there are nested MakeCallback's // then it should return early and not reach this code. - CHECK_EQ(env->current_async_id(), asyncContext.async_id); - CHECK_EQ(env->trigger_id(), asyncContext.trigger_async_id); + CHECK_EQ(env_->current_async_id(), 0); + CHECK_EQ(env_->trigger_id(), 0); - Local process = env->process_object(); + Local process = env_->process_object(); if (tick_info->length() == 0) { tick_info->set_index(0); - return ret; + return; + } + + CHECK_EQ(env_->current_async_id(), 0); + CHECK_EQ(env_->trigger_id(), 0); + + if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { + failed_ = true; + } +} + +MaybeLocal InternalMakeCallback(Environment* env, + Local recv, + const Local callback, + int argc, + Local argv[], + async_context asyncContext) { + InternalCallbackScope scope(env, recv, asyncContext); + if (scope.Failed()) { + return Undefined(env->isolate()); + } + + MaybeLocal ret; + + { + ret = callback->Call(env->context(), recv, argc, argv); + + if (ret.IsEmpty()) { + // NOTE: For backwards compatibility with public API we return Undefined() + // if the top level call threw. + scope.MarkAsFailed(); + return scope.IsInnerMakeCallback() ? ret : Undefined(env->isolate()); + } } - if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { + scope.Close(); + if (scope.Failed()) { return Undefined(env->isolate()); } @@ -1442,8 +1511,8 @@ MaybeLocal MakeCallback(Isolate* isolate, // the two contexts need not be the same. Environment* env = Environment::GetCurrent(callback->CreationContext()); Context::Scope context_scope(env->context()); - return MakeCallback(env, recv.As(), callback, argc, argv, - asyncContext); + return InternalMakeCallback(env, recv, callback, + argc, argv, asyncContext); } diff --git a/src/node.h b/src/node.h index 5fe7bde428..6558a9ee6d 100644 --- a/src/node.h +++ b/src/node.h @@ -570,6 +570,36 @@ NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate, NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate, async_context asyncContext); +class InternalCallbackScope; + +/* This class works like `MakeCallback()` in that it sets up a specific + * asyncContext as the current one and informs the async_hooks and domains + * modules that this context is currently active. + * + * `MakeCallback()` is a wrapper around this class as well as + * `Function::Call()`. Either one of these mechanisms needs to be used for + * top-level calls into JavaScript (i.e. without any existing JS stack). + * + * This object should be stack-allocated to ensure that it is contained in a + * valid HandleScope. + */ +class NODE_EXTERN CallbackScope { + public: + CallbackScope(v8::Isolate* isolate, + v8::Local resource, + async_context asyncContext); + ~CallbackScope(); + + private: + InternalCallbackScope* private_; + v8::TryCatch try_catch_; + + void operator=(const CallbackScope&) = delete; + void operator=(CallbackScope&&) = delete; + CallbackScope(const CallbackScope&) = delete; + CallbackScope(CallbackScope&&) = delete; +}; + /* An API specific to emit before/after callbacks is unnecessary because * MakeCallback will automatically call them for you. * @@ -659,6 +689,16 @@ class AsyncResource { async_id get_trigger_async_id() const { return async_context_.trigger_async_id; } + + protected: + class CallbackScope : public node::CallbackScope { + public: + explicit CallbackScope(AsyncResource* res) + : node::CallbackScope(res->isolate_, + res->resource_.Get(res->isolate_), + res->async_context_) {} + }; + private: v8::Isolate* isolate_; v8::Persistent resource_; diff --git a/src/node_internals.h b/src/node_internals.h index 9b6fae9d6a..9371d442ad 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -286,6 +286,14 @@ static v8::MaybeLocal New(Environment* env, } } // namespace Buffer +v8::MaybeLocal InternalMakeCallback( + Environment* env, + v8::Local recv, + const v8::Local callback, + int argc, + v8::Local argv[], + async_context asyncContext); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/addons/callback-scope/binding.cc b/test/addons/callback-scope/binding.cc new file mode 100644 index 0000000000..31317e5dae --- /dev/null +++ b/test/addons/callback-scope/binding.cc @@ -0,0 +1,39 @@ +#include "node.h" +#include "v8.h" + +#include +#include + +namespace { + +void RunInCallbackScope(const v8::FunctionCallbackInfo& args) { + v8::Isolate* isolate = args.GetIsolate(); + + assert(args.Length() == 4); + assert(args[0]->IsObject()); + assert(args[1]->IsNumber()); + assert(args[2]->IsNumber()); + assert(args[3]->IsFunction()); + + node::async_context asyncContext = { + args[1].As()->Value(), + args[2].As()->Value() + }; + + node::CallbackScope scope(isolate, args[0].As(), asyncContext); + v8::Local fn = args[3].As(); + + v8::MaybeLocal ret = + fn->Call(isolate->GetCurrentContext(), args[0], 0, nullptr); + + if (!ret.IsEmpty()) + args.GetReturnValue().Set(ret.ToLocalChecked()); +} + +void Initialize(v8::Local exports) { + NODE_SET_METHOD(exports, "runInCallbackScope", RunInCallbackScope); +} + +} // namespace + +NODE_MODULE(binding, Initialize) diff --git a/test/addons/callback-scope/binding.gyp b/test/addons/callback-scope/binding.gyp new file mode 100644 index 0000000000..7ede63d94a --- /dev/null +++ b/test/addons/callback-scope/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons/callback-scope/test-async-hooks.js b/test/addons/callback-scope/test-async-hooks.js new file mode 100644 index 0000000000..94b53efc53 --- /dev/null +++ b/test/addons/callback-scope/test-async-hooks.js @@ -0,0 +1,22 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); +const { runInCallbackScope } = require(`./build/${common.buildType}/binding`); + +let insideHook = false; +async_hooks.createHook({ + before: common.mustCall((id) => { + assert.strictEqual(id, 1000); + insideHook = true; + }), + after: common.mustCall((id) => { + assert.strictEqual(id, 1000); + insideHook = false; + }) +}).enable(); + +runInCallbackScope({}, 1000, 1000, () => { + assert(insideHook); +}); diff --git a/test/addons/callback-scope/test.js b/test/addons/callback-scope/test.js new file mode 100644 index 0000000000..2f2efe5f47 --- /dev/null +++ b/test/addons/callback-scope/test.js @@ -0,0 +1,17 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const { runInCallbackScope } = require(`./build/${common.buildType}/binding`); + +assert.strictEqual(runInCallbackScope({}, 0, 0, () => 42), 42); + +{ + process.once('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, 'foo'); + })); + + runInCallbackScope({}, 0, 0, () => { + throw new Error('foo'); + }); +}