src: clean up MultiIsolatePlatform interface

src/env.cc

@@ -81,8 +81,6 @@ IsolateData::IsolateData(Isolate* isolate,
       uses_node_allocator_(allocator_ == node_allocator_),
       platform_(platform) {
   CHECK_NOT_NULL(allocator_);
-  if (platform_ != nullptr)
-    platform_->RegisterIsolate(isolate_, event_loop);
 
   options_.reset(
       new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
@@ -134,12 +132,6 @@ IsolateData::IsolateData(Isolate* isolate,
 #undef V
 }
 
-IsolateData::~IsolateData() {
-  if (platform_ != nullptr)
-    platform_->UnregisterIsolate(isolate_);
-}
-
-
 void InitThreadLocalOnce() {
   CHECK_EQ(0, uv_key_create(&Environment::thread_local_env));
 }

src/env.h

@@ -399,7 +399,6 @@ class IsolateData {
               uv_loop_t* event_loop,
               MultiIsolatePlatform* platform = nullptr,
               ArrayBufferAllocator* node_allocator = nullptr);
-  ~IsolateData();
   inline uv_loop_t* event_loop() const;
   inline MultiIsolatePlatform* platform() const;
   inline std::shared_ptr<PerIsolateOptions> options();

src/node.h

@@ -228,10 +228,22 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
   virtual void DrainTasks(v8::Isolate* isolate) = 0;
   virtual void CancelPendingDelayedTasks(v8::Isolate* isolate) = 0;
 
-  // These will be called by the `IsolateData` creation/destruction functions.
+  // This needs to be called between the calls to `Isolate::Allocate()` and
+  // `Isolate::Initialize()`, so that initialization can already start
+  // using the platform.
+  // When using `NewIsolate()`, this is taken care of by that function.
+  // This function may only be called once per `Isolate`.
   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`.
   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,
+  // happen asynchronously.
+  virtual void AddIsolateFinishedCallback(v8::Isolate* isolate,
+                                          void (*callback)(void*),
+                                          void* data) = 0;
 };
 
 // Creates a new isolate with Node.js-specific settings.

src/node_platform.cc

@@ -260,6 +260,11 @@ PerIsolatePlatformData::~PerIsolatePlatformData() {
   Shutdown();
 }
 
+void PerIsolatePlatformData::AddShutdownCallback(void (*callback)(void*),
+                                                 void* data) {
+  shutdown_callbacks_.emplace_back(ShutdownCallback { callback, data });
+}
+
 void PerIsolatePlatformData::Shutdown() {
   if (flush_tasks_ == nullptr)
     return;
@@ -268,21 +273,19 @@ void PerIsolatePlatformData::Shutdown() {
   CHECK_NULL(foreground_tasks_.Pop());
   CancelPendingDelayedTasks();
 
+  ShutdownCbList* copy = new ShutdownCbList(std::move(shutdown_callbacks_));
+  flush_tasks_->data = copy;
   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);
   });
   flush_tasks_ = nullptr;
 }
 
-void PerIsolatePlatformData::ref() {
-  ref_count_++;
-}
-
-int PerIsolatePlatformData::unref() {
-  return --ref_count_;
-}
-
 NodePlatform::NodePlatform(int thread_pool_size,
                            TracingController* tracing_controller) {
   if (tracing_controller) {
@@ -297,23 +300,29 @@ NodePlatform::NodePlatform(int thread_pool_size,
 void NodePlatform::RegisterIsolate(Isolate* isolate, uv_loop_t* loop) {
   Mutex::ScopedLock lock(per_isolate_mutex_);
   std::shared_ptr<PerIsolatePlatformData> existing = per_isolate_[isolate];
-  if (existing) {
-    CHECK_EQ(loop, existing->event_loop());
-    existing->ref();
-  } else {
-    per_isolate_[isolate] =
-        std::make_shared<PerIsolatePlatformData>(isolate, loop);
-  }
+  CHECK(!existing);
+  per_isolate_[isolate] =
+      std::make_shared<PerIsolatePlatformData>(isolate, loop);
 }
 
 void NodePlatform::UnregisterIsolate(Isolate* isolate) {
   Mutex::ScopedLock lock(per_isolate_mutex_);
   std::shared_ptr<PerIsolatePlatformData> existing = per_isolate_[isolate];
   CHECK(existing);
-  if (existing->unref() == 0) {
-    existing->Shutdown();
-    per_isolate_.erase(isolate);
+  existing->Shutdown();
+  per_isolate_.erase(isolate);
+}
+
+void NodePlatform::AddIsolateFinishedCallback(Isolate* isolate,
+                                              void (*cb)(void*), void* data) {
+  Mutex::ScopedLock lock(per_isolate_mutex_);
+  auto it = per_isolate_.find(isolate);
+  if (it == per_isolate_.end()) {
+    CHECK(it->second);
+    cb(data);
+    return;
   }
+  it->second->AddShutdownCallback(cb, data);
 }
 
 void NodePlatform::Shutdown() {

src/node_platform.h

@@ -64,11 +64,9 @@ class PerIsolatePlatformData :
                        double delay_in_seconds) override;
   bool IdleTasksEnabled() override { return false; }
 
+  void AddShutdownCallback(void (*callback)(void*), void* data);
   void Shutdown();
 
-  void ref();
-  int unref();
-
   // Returns true if work was dispatched or executed. New tasks that are
   // posted during flushing of the queue are postponed until the next
   // flushing.
@@ -84,7 +82,13 @@ class PerIsolatePlatformData :
   static void RunForegroundTask(std::unique_ptr<v8::Task> task);
   static void RunForegroundTask(uv_timer_t* timer);
 
-  int ref_count_ = 1;
+  struct ShutdownCallback {
+    void (*cb)(void*);
+    void* data;
+  };
+  typedef std::vector<ShutdownCallback> ShutdownCbList;
+  ShutdownCbList shutdown_callbacks_;
+
   uv_loop_t* const loop_;
   uv_async_t* flush_tasks_ = nullptr;
   TaskQueue<v8::Task> foreground_tasks_;
@@ -145,6 +149,8 @@ class NodePlatform : public MultiIsolatePlatform {
 
   void RegisterIsolate(v8::Isolate* isolate, uv_loop_t* loop) override;
   void UnregisterIsolate(v8::Isolate* isolate) override;
+  void AddIsolateFinishedCallback(v8::Isolate* isolate,
+                                  void (*callback)(void*), void* data) override;
 
   std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(
       v8::Isolate* isolate) override;

src/node_worker.cc

@@ -186,16 +186,20 @@ class WorkerThreadData {
 
     w_->platform_->CancelPendingDelayedTasks(isolate);
 
+    bool platform_finished = false;
+
     isolate_data_.reset();
+
+    w_->platform_->AddIsolateFinishedCallback(isolate, [](void* data) {
+      *static_cast<bool*>(data) = true;
+    }, &platform_finished);
     w_->platform_->UnregisterIsolate(isolate);
 
     isolate->Dispose();
 
-    // Need to run the loop twice more to close the platform's uv_async_t
-    // TODO(addaleax): It would be better for the platform itself to provide
-    // some kind of notification when it has fully cleaned up.
-    uv_run(&loop_, UV_RUN_ONCE);
-    uv_run(&loop_, UV_RUN_ONCE);
+    // Wait until the platform has cleaned up all relevant resources.
+    while (!platform_finished)
+      uv_run(&loop_, UV_RUN_ONCE);
 
     CheckedUvLoopClose(&loop_);
   }