Fix isolate leak on teardown (#60)

src/v8_py_frontend/cancelable_task_runner.cc

@@ -1,64 +1,52 @@
 #include "cancelable_task_runner.h"
 
 #include <cstdint>
+#include <future>
 #include <memory>
-#include <mutex>
 #include <utility>
+#include <vector>
 #include "id_maker.h"
 #include "isolate_manager.h"
 
 namespace MiniRacer {
 
-CancelableTaskState::CancelableTaskState() : state_(State::kNotStarted) {}
-
-void CancelableTaskState::Cancel(IsolateManager* isolate_manager) {
-  const std::lock_guard<std::mutex> lock(mutex_);
-
-  if (state_ == State::kCanceled || state_ == State::kCompleted) {
-    return;
-  }
-
-  if (state_ == State::kRunning) {
-    isolate_manager->TerminateOngoingTask();
-  }
+void CancelableTaskBase::SetFuture(std::future<void> fut) {
+  future_promise_.set_value(std::move(fut));
+}
 
-  state_ = State::kCanceled;
+void CancelableTaskBase::Await() {
+  future_promise_.get_future().get();
 }
 
-auto CancelableTaskState::SetRunningIfNotCanceled() -> bool {
-  const std::lock_guard<std::mutex> lock(mutex_);
-  if (state_ == State::kCanceled) {
-    return false;
-  }
+CancelableTaskManager::CancelableTaskManager(IsolateManager* isolate_manager)
+    : isolate_manager_(isolate_manager),
+      task_id_maker_(std::make_shared<IdMaker<CancelableTaskBase>>()) {}
 
-  state_ = State::kRunning;
-  return true;
-}
+CancelableTaskManager::~CancelableTaskManager() {
+  // Normally, completed or canceled tasks will clean themselves out of the
+  // IdMaker. However, some tasks may still be pending upon teardown. Let's
+  // explicitly cancel and await any stragglers:
+  const std::vector<std::shared_ptr<CancelableTaskBase>> pending_tasks =
+      task_id_maker_->GetObjects();
 
-auto CancelableTaskState::SetCompleteIfNotCanceled() -> bool {
-  const std::lock_guard<std::mutex> lock(mutex_);
-  if (state_ == State::kCanceled) {
-    return false;
+  for (const auto& task : pending_tasks) {
+    task->Cancel(isolate_manager_);
   }
 
-  state_ = State::kCompleted;
-  return true;
+  for (const auto& task : pending_tasks) {
+    task->Await();
+  }
 }
 
-CancelableTaskRunner::CancelableTaskRunner(
-    std::shared_ptr<IsolateManager> isolate_manager)
-    : isolate_manager_(std::move(isolate_manager)),
-      task_id_maker_(std::make_shared<IdMaker<CancelableTaskState>>()) {}
-
-void CancelableTaskRunner::Cancel(uint64_t task_id) {
-  const std::shared_ptr<CancelableTaskState> task_state =
+void CancelableTaskManager::Cancel(uint64_t task_id) {
+  const std::shared_ptr<CancelableTaskBase> task =
       task_id_maker_->GetObject(task_id);
-  if (!task_state) {
+  if (!task) {
     // No such task found. This will commonly happen if a task is canceled
     // after it has already completed.
     return;
   }
-  task_state->Cancel(isolate_manager_.get());
+  task->Cancel(isolate_manager_);
 }
 
 }  // end namespace MiniRacer

src/v8_py_frontend/cancelable_task_runner.h

@@ -2,6 +2,7 @@
 #define INCLUDE_MINI_RACER_CANCELABLE_TASK_RUNNER_H
 
 #include <cstdint>
+#include <future>
 #include <memory>
 #include <mutex>
 #include <utility>
@@ -11,32 +12,39 @@
 
 namespace MiniRacer {
 
-/** Keeps track of status of a cancelable task. */
-class CancelableTaskState {
+class CancelableTaskBase {
  public:
-  explicit CancelableTaskState();
+  CancelableTaskBase() = default;
+  virtual ~CancelableTaskBase() = default;
 
-  void Cancel(IsolateManager* isolate_manager);
+  CancelableTaskBase(const CancelableTaskBase&) = delete;
+  auto operator=(const CancelableTaskBase&) -> CancelableTaskBase& = delete;
+  CancelableTaskBase(CancelableTaskBase&&) = delete;
+  auto operator=(CancelableTaskBase&& other) -> CancelableTaskBase& = delete;
 
-  auto SetRunningIfNotCanceled() -> bool;
+  virtual void Cancel(IsolateManager* isolate_manager) = 0;
 
-  auto SetCompleteIfNotCanceled() -> bool;
+  void SetFuture(std::future<void> fut);
+  void Await();
 
  private:
-  enum State : std::uint8_t {
-    kNotStarted = 0,
-    kRunning = 1,
-    kCompleted = 2,
-    kCanceled = 3
-  } state_;
-  std::mutex mutex_;
+  // A promise which yields a future which blocks until the underlying task
+  // is complete.
+  std::promise<std::future<void>> future_promise_;
 };
 
 /** Grafts a concept of cancelable tasks on top of an IsolateManager. */
-class CancelableTaskRunner {
+class CancelableTaskManager {
  public:
-  explicit CancelableTaskRunner(
-      std::shared_ptr<IsolateManager> isolate_manager);
+  explicit CancelableTaskManager(IsolateManager* isolate_manager);
+  ~CancelableTaskManager();
+
+  CancelableTaskManager(const CancelableTaskManager&) = delete;
+  auto operator=(const CancelableTaskManager&) -> CancelableTaskManager& =
+                                                      delete;
+  CancelableTaskManager(CancelableTaskManager&&) = delete;
+  auto operator=(CancelableTaskManager&& other) -> CancelableTaskManager& =
+                                                       delete;
 
   /**
    * Schedule the given runnable.
@@ -48,7 +56,7 @@ class CancelableTaskRunner {
    * those two functions will be called.)
    *
    * We split up these into separate functors to discourage side-channel passing
-   * of result data; the caller should follow the CancelableTaskRunner's view
+   * of result data; the caller should follow the CancelableTaskManager's view
    * regarding whether the task was completed or canceled.
    */
   template <typename Runnable, typename OnCompleted, typename OnCanceled>
@@ -59,62 +67,98 @@ class CancelableTaskRunner {
   void Cancel(uint64_t task_id);
 
  private:
-  std::shared_ptr<IsolateManager> isolate_manager_;
-  std::shared_ptr<IdMaker<CancelableTaskState>> task_id_maker_;
+  IsolateManager* isolate_manager_;
+  std::shared_ptr<IdMaker<CancelableTaskBase>> task_id_maker_;
 };
 
 template <typename Runnable, typename OnCompleted, typename OnCanceled>
-class CancelableTask {
+class CancelableTask : public CancelableTaskBase {
  public:
-  CancelableTask(Runnable runnable,
-                 OnCompleted on_completed,
-                 OnCanceled on_canceled,
-                 std::shared_ptr<IdMaker<CancelableTaskState>> task_id_maker);
+  explicit CancelableTask(Runnable runnable,
+                          OnCompleted on_completed,
+                          OnCanceled on_canceled);
 
   void Run(v8::Isolate* isolate);
-
-  auto TaskId() -> uint64_t;
+  void Cancel(IsolateManager* isolate_manager) override;
 
  private:
   Runnable runnable_;
   OnCompleted on_completed_;
   OnCanceled on_canceled_;
-  std::shared_ptr<CancelableTaskState> task_state_;
-  IdHolder<CancelableTaskState> task_id_holder_;
+
+  std::mutex mutex_;
+  enum State : std::uint8_t {
+    kNotStarted = 0,
+    kRunning = 1,
+    kCompleted = 2,
+    kCanceled = 3
+  } state_;
 };
 
+template <typename Runnable, typename OnCompleted, typename OnCanceled>
+inline auto CancelableTaskManager::Schedule(Runnable runnable,
+                                            OnCompleted on_completed,
+                                            OnCanceled on_canceled)
+    -> uint64_t {
+  auto task =
+      std::make_shared<CancelableTask<Runnable, OnCompleted, OnCanceled>>(
+          std::move(runnable), std::move(on_completed), std::move(on_canceled));
+
+  IdHolder<CancelableTaskBase> task_id_holder(task, task_id_maker_);
+
+  const uint64_t task_id = task_id_holder.GetId();
+
+  std::future<void> fut = isolate_manager_->Run(
+      [holder = std::move(task_id_holder), task](v8::Isolate* isolate) mutable {
+        task->Run(isolate);
+      });
+
+  task->SetFuture(std::move(fut));
+
+  return task_id;
+}
+
 template <typename Runnable, typename OnCompleted, typename OnCanceled>
 inline CancelableTask<Runnable, OnCompleted, OnCanceled>::CancelableTask(
     Runnable runnable,
     OnCompleted on_completed,
-    OnCanceled on_canceled,
-    std::shared_ptr<IdMaker<CancelableTaskState>> task_id_maker)
+    OnCanceled on_canceled)
     : runnable_(std::move(runnable)),
       on_completed_(std::move(on_completed)),
       on_canceled_(std::move(on_canceled)),
-      task_state_(std::make_shared<CancelableTaskState>()),
-      task_id_holder_(task_state_, std::move(task_id_maker)) {}
-
-template <typename Runnable, typename OnCompleted, typename OnCanceled>
-inline auto CancelableTask<Runnable, OnCompleted, OnCanceled>::TaskId()
-    -> uint64_t {
-  return task_id_holder_.GetId();
-}
+      state_(State::kNotStarted) {}
 
 template <typename Runnable, typename OnCompleted, typename OnCanceled>
 inline void CancelableTask<Runnable, OnCompleted, OnCanceled>::Run(
     v8::Isolate* isolate) {
-  if (!task_state_->SetRunningIfNotCanceled()) {
-    // Canceled before we started the task.
-    on_canceled_({});
+  bool was_canceled_before_run = false;
+  {
+    const std::lock_guard<std::mutex> lock(mutex_);
+    if (state_ == State::kCanceled) {
+      was_canceled_before_run = true;
+    } else {
+      state_ = State::kRunning;
+    }
+  }
 
+  if (was_canceled_before_run) {
+    on_canceled_({});
     return;
   }
 
   auto result = runnable_(isolate);
 
-  if (!task_state_->SetCompleteIfNotCanceled()) {
-    // Canceled while running.
+  bool was_canceled_during_run = false;
+  {
+    const std::lock_guard<std::mutex> lock(mutex_);
+    if (state_ == State::kCanceled) {
+      was_canceled_during_run = true;
+    } else {
+      state_ = State::kCompleted;
+    }
+  }
+
+  if (was_canceled_during_run) {
     // Note that we might actually complete our call to runnable_() and still
     // report the task as canceled, if a cancel request comes in right at the
     // end. Or we might have been halfway through running some JavaScript code
@@ -134,26 +178,19 @@ inline void CancelableTask<Runnable, OnCompleted, OnCanceled>::Run(
 }
 
 template <typename Runnable, typename OnCompleted, typename OnCanceled>
-inline auto CancelableTaskRunner::Schedule(Runnable runnable,
-                                           OnCompleted on_completed,
-                                           OnCanceled on_canceled) -> uint64_t {
-  auto task =
-      std::make_unique<CancelableTask<Runnable, OnCompleted, OnCanceled>>(
-          std::move(runnable), std::move(on_completed), std::move(on_canceled),
-          task_id_maker_);
+inline void CancelableTask<Runnable, OnCompleted, OnCanceled>::Cancel(
+    IsolateManager* isolate_manager) {
+  const std::lock_guard<std::mutex> lock(mutex_);
 
-  uint64_t task_id = task->TaskId();
+  if (state_ == State::kCanceled || state_ == State::kCompleted) {
+    return;
+  }
 
-  // clang-tidy-18 gives a strange warning about a memory leak here, which would
-  // seem to me to be impossible since we're only using a unique_ptr. (And
-  // testing demonstrates we don't leak here.)
-  // NOLINTBEGIN(clang-analyzer-cplusplus.NewDeleteLeaks)
-  isolate_manager_->Run([task = std::move(task)](v8::Isolate* isolate) mutable {
-    task->Run(isolate);
-  });
-  // NOLINTEND(clang-analyzer-cplusplus.NewDeleteLeaks)
+  if (state_ == State::kRunning) {
+    isolate_manager->TerminateOngoingTask();
+  }
 
-  return task_id;
+  state_ = State::kCanceled;
 }
 
 }  // end namespace MiniRacer

src/v8_py_frontend/code_evaluator.cc

@@ -9,21 +9,18 @@
 #include <v8-primitive.h>
 #include <v8-script.h>
 #include <v8-value.h>
-#include <memory>
-#include <utility>
 #include "binary_value.h"
 #include "context_holder.h"
 #include "isolate_memory_monitor.h"
 
 namespace MiniRacer {
 
-CodeEvaluator::CodeEvaluator(
-    std::shared_ptr<ContextHolder> context,
-    std::shared_ptr<BinaryValueFactory> bv_factory,
-    std::shared_ptr<IsolateMemoryMonitor> memory_monitor)
-    : context_(std::move(context)),
-      bv_factory_(std::move(bv_factory)),
-      memory_monitor_(std::move(memory_monitor)) {}
+CodeEvaluator::CodeEvaluator(ContextHolder* context,
+                             BinaryValueFactory* bv_factory,
+                             IsolateMemoryMonitor* memory_monitor)
+    : context_(context),
+      bv_factory_(bv_factory),
+      memory_monitor_(memory_monitor) {}
 
 auto CodeEvaluator::Eval(v8::Isolate* isolate,
                          BinaryValue* code_ptr) -> BinaryValue::Ptr {

src/v8_py_frontend/code_evaluator.h

@@ -5,7 +5,6 @@
 #include <v8-isolate.h>
 #include <v8-local-handle.h>
 #include <v8-persistent-handle.h>
-#include <memory>
 #include "binary_value.h"
 #include "context_holder.h"
 #include "isolate_memory_monitor.h"
@@ -15,16 +14,16 @@ namespace MiniRacer {
 /** Parse and run arbitrary scripts within an isolate. */
 class CodeEvaluator {
  public:
-  CodeEvaluator(std::shared_ptr<ContextHolder> context,
-                std::shared_ptr<BinaryValueFactory> bv_factory,
-                std::shared_ptr<IsolateMemoryMonitor> memory_monitor);
+  CodeEvaluator(ContextHolder* context,
+                BinaryValueFactory* bv_factory,
+                IsolateMemoryMonitor* memory_monitor);
 
   auto Eval(v8::Isolate* isolate, BinaryValue* code_ptr) -> BinaryValue::Ptr;
 
  private:
-  std::shared_ptr<ContextHolder> context_;
-  std::shared_ptr<BinaryValueFactory> bv_factory_;
-  std::shared_ptr<IsolateMemoryMonitor> memory_monitor_;
+  ContextHolder* context_;
+  BinaryValueFactory* bv_factory_;
+  IsolateMemoryMonitor* memory_monitor_;
 };
 
 }  // end namespace MiniRacer