inspector: properly shut down uv_async_t

Closing in the Agent destructor is too late, because that happens
when the Environment is destroyed, not when libuv handles are closed.

This fixes a situation in which the same libuv loop is re-used for
multiple Environment instances sequentially, e.g. in our cctest.

PR-URL: https://github.com/nodejs/node/pull/30612
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

1 files changed, 21 insertions, 8 deletions

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
index f13e68c067..fe60c82cb5 100644
--- a/src/inspector_agent.cc
+++ b/src/inspector_agent.cc
@@ -60,6 +60,8 @@ static uv_async_t start_io_thread_async;
 // This is just an additional check to make sure start_io_thread_async
 // is not accidentally re-used or used when uninitialized.
 static std::atomic_bool start_io_thread_async_initialized { false };
+// Protects the Agent* stored in start_io_thread_async.data.
+static Mutex start_io_thread_async_mutex;
 
 class StartIoTask : public Task {
  public:
@@ -97,6 +99,8 @@ static void StartIoThreadWakeup(int signo, siginfo_t* info, void* ucontext) {
 inline void* StartIoThreadMain(void* unused) {
   for (;;) {
     uv_sem_wait(&start_io_thread_semaphore);
+    Mutex::ScopedLock lock(start_io_thread_async_mutex);
+
     CHECK(start_io_thread_async_initialized);
     Agent* agent = static_cast<Agent*>(start_io_thread_async.data);
     if (agent != nullptr)
@@ -157,6 +161,7 @@ static int StartDebugSignalHandler() {
 
 #ifdef _WIN32
 DWORD WINAPI StartIoThreadProc(void* arg) {
+  Mutex::ScopedLock lock(start_io_thread_async_mutex);
   CHECK(start_io_thread_async_initialized);
   Agent* agent = static_cast<Agent*>(start_io_thread_async.data);
   if (agent != nullptr)
@@ -748,14 +753,7 @@ Agent::Agent(Environment* env)
       debug_options_(env->options()->debug_options()),
       host_port_(env->inspector_host_port()) {}
 
-Agent::~Agent() {
-  if (start_io_thread_async.data == this) {
-    CHECK(start_io_thread_async_initialized.exchange(false));
-    start_io_thread_async.data = nullptr;
-    // This is global, will never get freed
-    uv_close(reinterpret_cast<uv_handle_t*>(&start_io_thread_async), nullptr);
-  }
-}
+Agent::~Agent() {}
 
 bool Agent::Start(const std::string& path,
                   const DebugOptions& options,
@@ -768,6 +766,7 @@ bool Agent::Start(const std::string& path,
 
   client_ = std::make_shared<NodeInspectorClient>(parent_env_, is_main);
   if (parent_env_->owns_inspector()) {
+    Mutex::ScopedLock lock(start_io_thread_async_mutex);
     CHECK_EQ(start_io_thread_async_initialized.exchange(true), false);
     CHECK_EQ(0, uv_async_init(parent_env_->event_loop(),
                               &start_io_thread_async,
@@ -776,6 +775,20 @@ bool Agent::Start(const std::string& path,
     start_io_thread_async.data = this;
     // Ignore failure, SIGUSR1 won't work, but that should not block node start.
     StartDebugSignalHandler();
+
+    parent_env_->AddCleanupHook([](void* data) {
+      Environment* env = static_cast<Environment*>(data);
+
+      {
+        Mutex::ScopedLock lock(start_io_thread_async_mutex);
+        start_io_thread_async.data = nullptr;
+      }
+
+      // This is global, will never get freed
+      env->CloseHandle(&start_io_thread_async, [](uv_async_t*) {
+        CHECK(start_io_thread_async_initialized.exchange(false));
+      });
+    }, parent_env_);
   }
 
   AtExit(parent_env_, [](void* env) {