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: #15428 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matthew Loring <mattloring@google.com>
nodejs · Sep 21, 2017 · f27b5e4 · f27b5e4
1 parent 01c680b
commit f27b5e4
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 37 deletions.
diff --git 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> 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<v8::Object> object_;
-  Environment::AsyncCallbackScope callback_scope_;
-  bool failed_ = false;
-  bool pushed_ids_ = false;
-  bool closed_ = false;
-};
-
 CallbackScope::CallbackScope(Isolate* isolate,
                              Local<Object> object,
                              async_context asyncContext)
@@ -1381,17 +1357,21 @@ CallbackScope::~CallbackScope() {
 
 InternalCallbackScope::InternalCallbackScope(Environment* env,
                                              Local<Object> 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<Value> InternalMakeCallback(Environment* env,
                                        int argc,
                                        Local<Value> 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
@@ -294,8 +294,36 @@ v8::MaybeLocal<v8::Value> InternalMakeCallback(
     v8::Local<v8::Value> 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<v8::Object> 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<v8::Object> 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
@@ -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<Task*>(handle->data);
+static void RunForegroundTask(Task* task) {
+  Isolate* isolate = Isolate::GetCurrent();
+  HandleScope scope(isolate);
+  Environment* env = Environment::GetCurrent(isolate);
+  InternalCallbackScope cb_scope(env, Local<Object>(), { 0, 0 },
+                                 InternalCallbackScope::kAllowEmptyResource);
   task->Run();
   delete task;
+}
+
+static void RunForegroundTask(uv_timer_t* handle) {
+  Task* task = static_cast<Task*>(handle->data);
+  RunForegroundTask(task);
   uv_close(reinterpret_cast<uv_handle_t*>(handle), [](uv_handle_t* handle) {
     delete reinterpret_cast<uv_timer_t*>(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<uint64_t>(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
@@ -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.