From ce13d438193179d1374b6c3d33d32aa81d0048c5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 3 Nov 2019 13:00:24 +0100 Subject: [PATCH] src: use callback scope for main script This allows removing custom code for setting the current async ids and running nextTicks. PR-URL: https://github.com/nodejs/node/pull/30236 Reviewed-By: Franziska Hinkelmann Reviewed-By: Gus Caplan Reviewed-By: David Carlier Reviewed-By: James M Snell --- src/api/callback.cc | 11 ++++++----- src/node.cc | 9 ++------- src/node_internals.h | 11 ++++++++--- src/node_main_instance.cc | 10 +++++++--- src/node_process.h | 9 --------- src/node_task_queue.cc | 16 ---------------- src/node_worker.cc | 11 +++++++---- 7 files changed, 30 insertions(+), 47 deletions(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index 6d4e28e1d9070f..bced9bb7abe919 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -43,12 +43,13 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap) InternalCallbackScope::InternalCallbackScope(Environment* env, Local object, const async_context& asyncContext, - ResourceExpectation expect) + int flags) : env_(env), async_context_(asyncContext), object_(object), - callback_scope_(env) { - CHECK_IMPLIES(expect == kRequireResource, !object.IsEmpty()); + callback_scope_(env), + skip_hooks_(flags & kSkipAsyncHooks) { + CHECK_IMPLIES(!(flags & kAllowEmptyResource), !object.IsEmpty()); CHECK_NOT_NULL(env); if (!env->can_call_into_js()) { @@ -60,7 +61,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(Environment::GetCurrent(env->isolate()), env); - if (asyncContext.async_id != 0) { + if (asyncContext.async_id != 0 && !skip_hooks_) { // No need to check a return value because the application will exit if // an exception occurs. AsyncWrap::EmitBefore(env, asyncContext.async_id); @@ -89,7 +90,7 @@ void InternalCallbackScope::Close() { if (failed_) return; - if (async_context_.async_id != 0) { + if (async_context_.async_id != 0 && !skip_hooks_) { AsyncWrap::EmitAfter(env_, async_context_.async_id); } diff --git a/src/node.cc b/src/node.cc index 2a80c46a575acf..1982156c7ae0c4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -374,13 +374,8 @@ MaybeLocal StartExecution(Environment* env, const char* main_script_id) { ->GetFunction(env->context()) .ToLocalChecked()}; - Local result; - if (!ExecuteBootstrapper(env, main_script_id, ¶meters, &arguments) - .ToLocal(&result) || - !task_queue::RunNextTicksNative(env)) { - return MaybeLocal(); - } - return scope.Escape(result); + return scope.EscapeMaybe( + ExecuteBootstrapper(env, main_script_id, ¶meters, &arguments)); } MaybeLocal StartMainThreadExecution(Environment* env) { diff --git a/src/node_internals.h b/src/node_internals.h index 0fff2e6111e53f..a53d75546e3850 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -205,12 +205,16 @@ v8::MaybeLocal InternalMakeCallback( class InternalCallbackScope { public: - // Tell the constructor whether its `object` parameter may be empty or not. - enum ResourceExpectation { kRequireResource, kAllowEmptyResource }; + enum Flags { + // Tell the constructor whether its `object` parameter may be empty or not. + kAllowEmptyResource = 1, + // Indicates whether 'before' and 'after' hooks should be skipped. + kSkipAsyncHooks = 2 + }; InternalCallbackScope(Environment* env, v8::Local object, const async_context& asyncContext, - ResourceExpectation expect = kRequireResource); + int flags = 0); // Utility that can be used by AsyncWrap classes. explicit InternalCallbackScope(AsyncWrap* async_wrap); ~InternalCallbackScope(); @@ -224,6 +228,7 @@ class InternalCallbackScope { async_context async_context_; v8::Local object_; AsyncCallbackScope callback_scope_; + bool skip_hooks_; bool failed_ = false; bool pushed_ids_ = false; bool closed_ = false; diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 4ae838ad54d89d..212b7f33e190dd 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -18,6 +18,7 @@ using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::Locker; +using v8::Object; using v8::SealHandleScope; NodeMainInstance::NodeMainInstance(Isolate* isolate, @@ -112,10 +113,13 @@ int NodeMainInstance::Run() { if (exit_code == 0) { { - AsyncCallbackScope callback_scope(env.get()); - env->async_hooks()->push_async_ids(1, 0); + InternalCallbackScope callback_scope( + env.get(), + Local(), + { 1, 0 }, + InternalCallbackScope::kAllowEmptyResource | + InternalCallbackScope::kSkipAsyncHooks); LoadEnvironment(env.get()); - env->async_hooks()->pop_async_id(1); } env->set_trace_sync_io(env->options()->trace_sync_io); diff --git a/src/node_process.h b/src/node_process.h index 48d5aa704f71e9..5db7b004d6f939 100644 --- a/src/node_process.h +++ b/src/node_process.h @@ -34,15 +34,6 @@ v8::Maybe ProcessEmitDeprecationWarning(Environment* env, v8::MaybeLocal CreateProcessObject(Environment* env); void PatchProcessObject(const v8::FunctionCallbackInfo& args); -namespace task_queue { -// Handle any nextTicks added in the first tick of the program. -// We use the native version here for once so that any microtasks -// created by the main module is then handled from C++, and -// the call stack of the main script does not show up in the async error -// stack trace. -bool RunNextTicksNative(Environment* env); -} // namespace task_queue - } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #endif // SRC_NODE_PROCESS_H_ diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index ef1aff6cd4138d..f418a272470e2b 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -41,22 +41,6 @@ static void EnqueueMicrotask(const FunctionCallbackInfo& args) { isolate->EnqueueMicrotask(args[0].As()); } -// Should be in sync with runNextTicks in internal/process/task_queues.js -bool RunNextTicksNative(Environment* env) { - OnScopeLeave weakref_cleanup([&]() { env->RunWeakRefCleanup(); }); - - TickInfo* tick_info = env->tick_info(); - if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn()) - MicrotasksScope::PerformCheckpoint(env->isolate()); - if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn()) - return true; - - Local callback = env->tick_callback_function(); - CHECK(!callback.IsEmpty()); - return !callback->Call(env->context(), env->process_object(), 0, nullptr) - .IsEmpty(); -} - static void RunMicrotasks(const FunctionCallbackInfo& args) { MicrotasksScope::PerformCheckpoint(args.GetIsolate()); } diff --git a/src/node_worker.cc b/src/node_worker.cc index ab442a4a34a8c6..5c5c445e555aae 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -254,8 +254,13 @@ void Worker::Run() { env_->InitializeInspector(std::move(inspector_parent_handle_)); #endif HandleScope handle_scope(isolate_); - AsyncCallbackScope callback_scope(env_.get()); - env_->async_hooks()->push_async_ids(1, 0); + InternalCallbackScope callback_scope( + env_.get(), + Local(), + { 1, 0 }, + InternalCallbackScope::kAllowEmptyResource | + InternalCallbackScope::kSkipAsyncHooks); + if (!env_->RunBootstrapping().IsEmpty()) { CreateEnvMessagePort(env_.get()); if (is_stopped()) return; @@ -263,8 +268,6 @@ void Worker::Run() { USE(StartExecution(env_.get(), "internal/main/worker_thread")); } - env_->async_hooks()->pop_async_id(1); - Debug(this, "Loaded environment for worker %llu", thread_id_); }