src: make implementing CancelPendingDelayedTasks for platform optional

addaleax · MylesBorins · commit ed80c233cd87 · 2019-10-23T03:33:04.000-04:00
Fold `CancelPendingDelayedTasks()` into `UnregisterIsolate()` and make implementing it optional. It makes sense for these two operations to happen at the same time, so it is sufficient to provide a single operation instead of two separate ones. PR-URL: #30034 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shelley Vohr <codebytere@gmail.com>
diff --git a/src/node.h b/src/node.h
@@ -260,7 +260,11 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
   // flushing.
   virtual bool FlushForegroundTasks(v8::Isolate* isolate) = 0;
   virtual void DrainTasks(v8::Isolate* isolate) = 0;
-  virtual void CancelPendingDelayedTasks(v8::Isolate* isolate) = 0;
+
+  // TODO(addaleax): Remove this, it is unnecessary.
+  // This would currently be called before `UnregisterIsolate()` but will be
+  // folded into it in the future.
+  virtual void CancelPendingDelayedTasks(v8::Isolate* isolate);
 
   // This needs to be called between the calls to `Isolate::Allocate()` and
   // `Isolate::Initialize()`, so that initialization can already start
@@ -270,7 +274,8 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
   virtual void RegisterIsolate(v8::Isolate* isolate,
                                struct uv_loop_s* loop) = 0;
   // This needs to be called right before calling `Isolate::Dispose()`.
-  // This function may only be called once per `Isolate`.
+  // This function may only be called once per `Isolate`, and discard any
+  // pending delayed tasks scheduled for that isolate.
   virtual void UnregisterIsolate(v8::Isolate* isolate) = 0;
   // The platform should call the passed function once all state associated
   // with the given isolate has been cleaned up. This can, but does not have to,
diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
@@ -155,7 +155,6 @@ int NodeMainInstance::Run() {
   RunAtExit(env.get());
 
   per_process::v8_platform.DrainVMTasks(isolate_);
-  per_process::v8_platform.CancelVMTasks(isolate_);
 
 #if defined(LEAK_SANITIZER)
   __lsan_do_leak_check();
diff --git a/src/node_platform.cc b/src/node_platform.cc
@@ -285,22 +285,33 @@ void PerIsolatePlatformData::Shutdown() {
   // effectively deleting the tasks instead of running them.
   foreground_delayed_tasks_.PopAll();
   foreground_tasks_.PopAll();
+  scheduled_delayed_tasks_.clear();
 
-  CancelPendingDelayedTasks();
-
-  ShutdownCbList* copy = new ShutdownCbList(std::move(shutdown_callbacks_));
-  flush_tasks_->data = copy;
+  // Both destroying the scheduled_delayed_tasks_ lists and closing
+  // flush_tasks_ handle add tasks to the event loop. We keep a count of all
+  // non-closed handles, and when that reaches zero, we inform any shutdown
+  // callbacks that the platform is done as far as this Isolate is concerned.
+  self_reference_ = shared_from_this();
   uv_close(reinterpret_cast<uv_handle_t*>(flush_tasks_),
            [](uv_handle_t* handle) {
-    std::unique_ptr<ShutdownCbList> callbacks(
-        static_cast<ShutdownCbList*>(handle->data));
-    for (const auto& callback : *callbacks)
-      callback.cb(callback.data);
-    delete reinterpret_cast<uv_async_t*>(handle);
+    std::unique_ptr<uv_async_t> flush_tasks {
+        reinterpret_cast<uv_async_t*>(handle) };
+    PerIsolatePlatformData* platform_data =
+        static_cast<PerIsolatePlatformData*>(flush_tasks->data);
+    platform_data->DecreaseHandleCount();
+    platform_data->self_reference_.reset();
   });
   flush_tasks_ = nullptr;
 }
 
+void PerIsolatePlatformData::DecreaseHandleCount() {
+  CHECK_GE(uv_handle_count_, 1);
+  if (--uv_handle_count_ == 0) {
+    for (const auto& callback : shutdown_callbacks_)
+      callback.cb(callback.data);
+  }
+}
+
 NodePlatform::NodePlatform(int thread_pool_size,
                            TracingController* tracing_controller) {
   if (tracing_controller) {
@@ -382,10 +393,6 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) {
   delayed->platform_data->DeleteFromScheduledTasks(delayed);
 }
 
-void PerIsolatePlatformData::CancelPendingDelayedTasks() {
-  scheduled_delayed_tasks_.clear();
-}
-
 void NodePlatform::DrainTasks(Isolate* isolate) {
   std::shared_ptr<PerIsolatePlatformData> per_isolate = ForIsolate(isolate);
 
@@ -409,12 +416,15 @@ bool PerIsolatePlatformData::FlushForegroundTasksInternal() {
     // the delay is non-zero. This should not be a problem in practice.
     uv_timer_start(&delayed->timer, RunForegroundTask, delay_millis, 0);
     uv_unref(reinterpret_cast<uv_handle_t*>(&delayed->timer));
+    uv_handle_count_++;
 
     scheduled_delayed_tasks_.emplace_back(delayed.release(),
                                           [](DelayedTask* delayed) {
       uv_close(reinterpret_cast<uv_handle_t*>(&delayed->timer),
                [](uv_handle_t* handle) {
-        delete static_cast<DelayedTask*>(handle->data);
+        std::unique_ptr<DelayedTask> task {
+            static_cast<DelayedTask*>(handle->data) };
+        task->platform_data->DecreaseHandleCount();
       });
     });
   }
@@ -454,10 +464,6 @@ bool NodePlatform::FlushForegroundTasks(Isolate* isolate) {
   return ForIsolate(isolate)->FlushForegroundTasksInternal();
 }
 
-void NodePlatform::CancelPendingDelayedTasks(Isolate* isolate) {
-  ForIsolate(isolate)->CancelPendingDelayedTasks();
-}
-
 bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; }
 
 std::shared_ptr<v8::TaskRunner>
@@ -548,4 +554,6 @@ std::queue<std::unique_ptr<T>> TaskQueue<T>::PopAll() {
   return result;
 }
 
+void MultiIsolatePlatform::CancelPendingDelayedTasks(Isolate* isolate) {}
+
 }  // namespace node
diff --git a/src/node_platform.h b/src/node_platform.h
@@ -78,12 +78,12 @@ class PerIsolatePlatformData :
   // posted during flushing of the queue are postponed until the next
   // flushing.
   bool FlushForegroundTasksInternal();
-  void CancelPendingDelayedTasks();
 
   const uv_loop_t* event_loop() const { return loop_; }
 
  private:
   void DeleteFromScheduledTasks(DelayedTask* task);
+  void DecreaseHandleCount();
 
   static void FlushTasks(uv_async_t* handle);
   static void RunForegroundTask(std::unique_ptr<v8::Task> task);
@@ -95,14 +95,17 @@ class PerIsolatePlatformData :
   };
   typedef std::vector<ShutdownCallback> ShutdownCbList;
   ShutdownCbList shutdown_callbacks_;
+  // shared_ptr to self to keep this object alive during shutdown.
+  std::shared_ptr<PerIsolatePlatformData> self_reference_;
+  uint32_t uv_handle_count_ = 1;  // 1 = flush_tasks_
 
   uv_loop_t* const loop_;
   uv_async_t* flush_tasks_ = nullptr;
   TaskQueue<v8::Task> foreground_tasks_;
   TaskQueue<DelayedTask> foreground_delayed_tasks_;
 
   // Use a custom deleter because libuv needs to close the handle first.
-  typedef std::unique_ptr<DelayedTask, std::function<void(DelayedTask*)>>
+  typedef std::unique_ptr<DelayedTask, void(*)(DelayedTask*)>
       DelayedTaskPointer;
   std::vector<DelayedTaskPointer> scheduled_delayed_tasks_;
 };
@@ -137,7 +140,6 @@ class NodePlatform : public MultiIsolatePlatform {
   ~NodePlatform() override = default;
 
   void DrainTasks(v8::Isolate* isolate) override;
-  void CancelPendingDelayedTasks(v8::Isolate* isolate) override;
   void Shutdown();
 
   // v8::Platform implementation.
diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h
@@ -113,10 +113,6 @@ struct V8Platform {
     platform_->DrainTasks(isolate);
   }
 
-  inline void CancelVMTasks(v8::Isolate* isolate) {
-    platform_->CancelPendingDelayedTasks(isolate);
-  }
-
   inline void StartTracingAgent() {
     // Attach a new NodeTraceWriter only if this function hasn't been called
     // before.
@@ -150,7 +146,6 @@ struct V8Platform {
   inline void Initialize(int thread_pool_size) {}
   inline void Dispose() {}
   inline void DrainVMTasks(v8::Isolate* isolate) {}
-  inline void CancelVMTasks(v8::Isolate* isolate) {}
   inline void StartTracingAgent() {
     if (!per_process::cli_options->trace_event_categories.empty()) {
       fprintf(stderr,
diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -148,8 +148,6 @@ class WorkerThreadData {
       w_->isolate_ = nullptr;
     }
 
-    w_->platform_->CancelPendingDelayedTasks(isolate);
-
     bool platform_finished = false;
 
     isolate_data_.reset();

Original file line number	Diff line number	Diff line change
`@@ -148,8 +148,6 @@ class WorkerThreadData {`
`148`	`148`	`w_->isolate_ = nullptr;`
`149`	`149`	`}`
`150`	`150`
`151`		`- w_->platform_->CancelPendingDelayedTasks(isolate);`
`152`		`-`
`153`	`151`	`bool platform_finished = false;`
`154`	`152`
`155`	`153`	`isolate_data_.reset();`