From 494d53fa3cec82e4bca7c79a9f1d4d8368edd5d8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 15 Sep 2017 15:03:48 +0200 Subject: [PATCH] src: prepare platform for upstream V8 changes V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: https://github.com/nodejs/node/pull/15428 Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis Reviewed-By: Matthew Loring --- src/node.cc | 45 ++++++++++++++++---------------------------- src/node_internals.h | 28 +++++++++++++++++++++++++++ src/node_platform.cc | 30 ++++++++++++++++++++++------- src/node_platform.h | 3 ++- 4 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/node.cc b/src/node.cc index a234569e21df55..d6678dd967ea71 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1339,30 +1339,6 @@ 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; -}; - CallbackScope::CallbackScope(Isolate* isolate, Local object, async_context asyncContext) @@ -1381,17 +1357,21 @@ CallbackScope::~CallbackScope() { InternalCallbackScope::InternalCallbackScope(Environment* env, Local object, - const async_context& asyncContext) + const async_context& asyncContext, + ResourceExpectation expect) : env_(env), async_context_(asyncContext), object_(object), callback_scope_(env) { - CHECK(!object.IsEmpty()); + if (expect == kRequireResource) { + CHECK(!object.IsEmpty()); + } + HandleScope handle_scope(env->isolate()); // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); - if (env->using_domains()) { + if (env->using_domains() && !object_.IsEmpty()) { DomainEnter(env, object_); } @@ -1413,6 +1393,7 @@ InternalCallbackScope::~InternalCallbackScope() { void InternalCallbackScope::Close() { if (closed_) return; closed_ = true; + HandleScope handle_scope(env_->isolate()); if (pushed_ids_) env_->async_hooks()->pop_ids(async_context_.async_id); @@ -1423,7 +1404,7 @@ void InternalCallbackScope::Close() { AsyncWrap::EmitAfter(env_, async_context_.async_id); } - if (env_->using_domains()) { + if (env_->using_domains() && !object_.IsEmpty()) { DomainExit(env_, object_); } @@ -1463,6 +1444,7 @@ MaybeLocal InternalMakeCallback(Environment* env, int argc, Local argv[], async_context asyncContext) { + CHECK(!recv.IsEmpty()); InternalCallbackScope scope(env, recv, asyncContext); if (scope.Failed()) { return Undefined(env->isolate()); @@ -4726,9 +4708,14 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, do { uv_run(env.event_loop(), UV_RUN_DEFAULT); + v8_platform.DrainVMTasks(); + + more = uv_loop_alive(env.event_loop()); + if (more) + continue; + EmitBeforeExit(&env); - v8_platform.DrainVMTasks(); // Emit `beforeExit` if the loop became alive either after emitting // event, or after running some callbacks. more = uv_loop_alive(env.event_loop()); diff --git a/src/node_internals.h b/src/node_internals.h index 9371d442ad0ea5..fd8cc26a2881ca 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -294,8 +294,36 @@ v8::MaybeLocal InternalMakeCallback( v8::Local argv[], async_context asyncContext); +class InternalCallbackScope { + public: + // Tell the constructor whether its `object` parameter may be empty or not. + enum ResourceExpectation { kRequireResource, kAllowEmptyResource }; + InternalCallbackScope(Environment* env, + v8::Local object, + const async_context& asyncContext, + ResourceExpectation expect = kRequireResource); + ~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; +}; + } // namespace node + #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #endif // SRC_NODE_INTERNALS_H_ diff --git a/src/node_platform.cc b/src/node_platform.cc index 3d023114ad2691..320136c3f72159 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -1,10 +1,14 @@ #include "node_platform.h" +#include "node_internals.h" #include "util.h" namespace node { +using v8::HandleScope; using v8::Isolate; +using v8::Local; +using v8::Object; using v8::Platform; using v8::Task; using v8::TracingController; @@ -63,22 +67,33 @@ size_t NodePlatform::NumberOfAvailableBackgroundThreads() { return threads_.size(); } -static void RunForegroundTask(uv_timer_t* handle) { - Task* task = static_cast(handle->data); +static void RunForegroundTask(Task* task) { + Isolate* isolate = Isolate::GetCurrent(); + HandleScope scope(isolate); + Environment* env = Environment::GetCurrent(isolate); + InternalCallbackScope cb_scope(env, Local(), { 0, 0 }, + InternalCallbackScope::kAllowEmptyResource); task->Run(); delete task; +} + +static void RunForegroundTask(uv_timer_t* handle) { + Task* task = static_cast(handle->data); + RunForegroundTask(task); uv_close(reinterpret_cast(handle), [](uv_handle_t* handle) { delete reinterpret_cast(handle); }); } void NodePlatform::DrainBackgroundTasks() { - FlushForegroundTasksInternal(); - background_tasks_.BlockingDrain(); + while (FlushForegroundTasksInternal()) + background_tasks_.BlockingDrain(); } -void NodePlatform::FlushForegroundTasksInternal() { +bool NodePlatform::FlushForegroundTasksInternal() { + bool did_work = false; while (auto delayed = foreground_delayed_tasks_.Pop()) { + did_work = true; uint64_t delay_millis = static_cast(delayed->second + 0.5) * 1000; uv_timer_t* handle = new uv_timer_t(); @@ -91,9 +106,10 @@ void NodePlatform::FlushForegroundTasksInternal() { delete delayed; } while (Task* task = foreground_tasks_.Pop()) { - task->Run(); - delete task; + did_work = true; + RunForegroundTask(task); } + return did_work; } void NodePlatform::CallOnBackgroundThread(Task* task, diff --git a/src/node_platform.h b/src/node_platform.h index 668fcf28e40233..04927fccc3df66 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -39,7 +39,8 @@ class NodePlatform : public v8::Platform { virtual ~NodePlatform() {} void DrainBackgroundTasks(); - void FlushForegroundTasksInternal(); + // Returns true iff work was dispatched or executed. + bool FlushForegroundTasksInternal(); void Shutdown(); // v8::Platform implementation.