Coordinate inspector and transport shutdown

packages/react-native/ReactCxxPlatform/react/devsupport/inspector/Inspector.cpp

@@ -135,18 +135,36 @@ Inspector::Inspector(
       httpClientFactory_(std::move(httpClientFactory)) {}
 
 Inspector::~Inspector() noexcept {
-  if (pageId_) {
-    jsinspector_modern::getInspectorInstance().removePage(*pageId_);
-    taskDispatchThread_.runSync([weakThis = weak_from_this()]() {
-      if (auto strongThis = weakThis.lock()) {
-        strongThis->pageId_.reset();
-        strongThis->target_.reset();
-      }
-    });
+  shutdown();
+}
+
+void Inspector::shutdown() noexcept {
+  CHECK(!taskDispatchThread_.isOnThread());
+  if (shuttingDown_.exchange(true)) {
+    return;
   }
+  taskDispatchThread_.runSync([this]() {
+    taskDispatchThread_.cancelPendingTasks();
+    if (pageId_) {
+      jsinspector_modern::getInspectorInstance().removePage(*pageId_);
+      pageId_.reset();
+    }
+    if (packagerConnection_) {
+      packagerConnection_->closeQuietly();
+      packagerConnection_.reset();
+    }
+    target_.reset();
+    hostDelegate_.reset();
+  });
   taskDispatchThread_.quit();
 }
 
+void Inspector::runOnInspectorThreadSync(
+    TaskDispatchThread::TaskFn&& callback) {
+  CHECK(!shuttingDown_);
+  taskDispatchThread_.runSync(std::move(callback));
+}
+
 void Inspector::connectDebugger(const std::string& inspectorUrl) noexcept {
   invokeElsePost([weakThis = weak_from_this(), inspectorUrl]() {
     auto self = weakThis.lock();
@@ -205,11 +223,20 @@ void Inspector::ensureHostTarget(
 void Inspector::invokeElsePost(
     TaskDispatchThread::TaskFn&& callback,
     std::chrono::milliseconds delayMs) {
+  if (shuttingDown_) {
+    return;
+  }
   if (taskDispatchThread_.isOnThread() &&
       delayMs == std::chrono::milliseconds::zero()) {
     callback();
   } else {
-    taskDispatchThread_.runAsync(std::move(callback), delayMs);
+    taskDispatchThread_.runAsync(
+        [weakThis = weak_from_this(), callback = std::move(callback)]() {
+          if (auto self = weakThis.lock(); self && !self->shuttingDown_) {
+            callback();
+          }
+        },
+        delayMs);
   }
 }
 

packages/react-native/ReactCxxPlatform/react/devsupport/inspector/Inspector.h

@@ -14,6 +14,7 @@
 #include <react/http/IHttpClient.h>
 #include <react/http/IWebSocketClient.h>
 #include <react/threading/TaskDispatchThread.h>
+#include <atomic>
 #include <functional>
 #include <memory>
 #include <optional>
@@ -37,6 +38,11 @@ class Inspector : public InspectorThread, public std::enable_shared_from_this<In
   Inspector(Inspector &&other) = delete;
   Inspector &operator=(Inspector &&other) = delete;
 
+  // Call from the owner thread before releasing the last owning reference.
+  void shutdown() noexcept;
+
+  void runOnInspectorThreadSync(TaskDispatchThread::TaskFn &&callback);
+
   void connectDebugger(const std::string &inspectorUrl) noexcept;
 
   void ensureHostTarget(
@@ -50,6 +56,7 @@ class Inspector : public InspectorThread, public std::enable_shared_from_this<In
       TaskDispatchThread::TaskFn &&callback,
       std::chrono::milliseconds delayMs = std::chrono::milliseconds::zero()) override;
 
+  std::atomic<bool> shuttingDown_{false};
   TaskDispatchThread taskDispatchThread_{"InspectorThread"};
   std::string appName_;
   std::string deviceName_;

packages/react-native/ReactCxxPlatform/react/devsupport/inspector/tests/InspectorShutdownTests.cpp

@@ -0,0 +1,150 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <gtest/gtest.h>
+#include <jsinspector-modern/InspectorInterfaces.h>
+#include <react/devsupport/inspector/Inspector.h>
+#include <chrono>
+#include <thread>
+
+namespace facebook::react {
+
+class InspectorShutdownTest : public ::testing::Test {
+ protected:
+  struct SocketState {
+    IWebSocketClient::OnConnectCallback onConnect;
+    IWebSocketClient::OnClosedCallback onClosed;
+    IWebSocketClient::OnMessageCallback onMessage;
+    std::weak_ptr<Inspector> inspector;
+    std::thread::id closeThread;
+    bool ownedDuringClose{false};
+    int closes{0};
+  };
+
+  class Socket final : public IWebSocketClient {
+   public:
+    explicit Socket(std::shared_ptr<SocketState> state)
+        : state_(std::move(state)) {}
+
+    void setOnClosedCallback(OnClosedCallback&& callback) noexcept override {
+      state_->onClosed = std::move(callback);
+    }
+
+    void setOnMessageCallback(OnMessageCallback&& callback) noexcept override {
+      state_->onMessage = std::move(callback);
+    }
+
+    void connect(const std::string& /*url*/, OnConnectCallback&& callback)
+        override {
+      state_->onConnect = std::move(callback);
+    }
+
+    void close(const std::string& reason) override {
+      ++state_->closes;
+      state_->closeThread = std::this_thread::get_id();
+      state_->ownedDuringClose = !state_->inspector.expired();
+      if (state_->onClosed) {
+        state_->onClosed(reason);
+      }
+    }
+
+    void send(const std::string& /*message*/) override {}
+    void ping() override {}
+
+   private:
+    std::shared_ptr<SocketState> state_;
+  };
+
+  void SetUp() override {
+    socket = std::make_shared<SocketState>();
+    inspector = std::make_shared<Inspector>(
+        "shutdown-test",
+        "test-device",
+        [state = socket]() { return std::make_unique<Socket>(state); },
+        []() { return std::unique_ptr<IHttpClient>{}; });
+    socket->inspector = inspector;
+  }
+
+  void TearDown() override {
+    if (inspector) {
+      inspector->shutdown();
+    }
+  }
+
+  std::shared_ptr<SocketState> socket;
+  std::shared_ptr<Inspector> inspector;
+};
+
+TEST_F(InspectorShutdownTest, ShutdownDisposesPageAndSocketWhileOwned) {
+  auto pageCount = jsinspector_modern::getInspectorInstance().getPages().size();
+  inspector->ensureHostTarget({}, {});
+  EXPECT_NE(inspector->inspectorTarget(), nullptr);
+  inspector->connectDebugger("http://localhost:8081/inspector/device");
+  std::thread::id inspectorThread;
+  inspector->runOnInspectorThreadSync(
+      [&]() { inspectorThread = std::this_thread::get_id(); });
+  ASSERT_TRUE(socket->onConnect);
+  EXPECT_EQ(
+      jsinspector_modern::getInspectorInstance().getPages().size(),
+      pageCount + 1);
+
+  inspector->shutdown();
+
+  EXPECT_EQ(inspector->inspectorTarget(), nullptr);
+  EXPECT_EQ(
+      jsinspector_modern::getInspectorInstance().getPages().size(), pageCount);
+  EXPECT_EQ(socket->closes, 1);
+  EXPECT_EQ(socket->closeThread, inspectorThread);
+  EXPECT_TRUE(socket->ownedDuringClose);
+  EXPECT_EQ(inspector.use_count(), 1);
+  inspector->shutdown();
+  EXPECT_EQ(socket->closes, 1);
+  inspector.reset();
+  EXPECT_TRUE(socket->inspector.expired());
+}
+
+TEST_F(
+    InspectorShutdownTest,
+    ShutdownDropsDelayedOwnersAndRejectsLateCallbacks) {
+  InspectorThread& dispatch = *inspector;
+  bool ran = false;
+  dispatch.invokeElsePost(
+      [owner = inspector, &ran]() { ran = true; }, std::chrono::hours(1));
+
+  inspector->shutdown();
+
+  EXPECT_FALSE(ran);
+  EXPECT_EQ(inspector.use_count(), 1);
+  dispatch.invokeElsePost([&]() { ran = true; });
+  dispatch.invokeElsePost([&]() { ran = true; }, std::chrono::hours(1));
+  inspector->connectDebugger("http://localhost:8081/inspector/device");
+  EXPECT_FALSE(ran);
+  EXPECT_FALSE(socket->onConnect);
+  inspector.reset();
+  EXPECT_TRUE(socket->inspector.expired());
+}
+
+TEST_F(
+    InspectorShutdownTest,
+    LateTransportFailureCannotReconnectAfterShutdown) {
+  inspector->connectDebugger("http://localhost:8081/inspector/device");
+  inspector->runOnInspectorThreadSync([]() {});
+  ASSERT_TRUE(socket->onConnect);
+  auto onConnect = socket->onConnect;
+  auto onMessage = socket->onMessage;
+
+  inspector->shutdown();
+  onConnect(false, "late failure");
+  onMessage(R"({"event":"getPages"})");
+
+  EXPECT_EQ(socket->closes, 1);
+  EXPECT_EQ(inspector.use_count(), 1);
+  inspector.reset();
+  EXPECT_TRUE(socket->inspector.expired());
+}
+
+} // namespace facebook::react