RuntimeTarget refactor - introduce WeakList for Target→Agent refs

Summary:
Changelog: [Internal]

Replaces the copypasta'd `PageTarget::forEachSession`, `InstanceTarget::forEachAgent` and `RuntimeTarget::forEachAgent` with a shared utility class for managing a list of `weak_ptr`s.

In a `WeakList`, elements can only be added (`insert`), iterated over (`forEach`), and counted (`size`, `empty`). Conceptually, elements are automatically removed from the list as soon as they're destroyed, but internally, space is reclaimed *lazily*: the next time we have a reason to iterate over the underlying list, we delete any pointers that are found to be null.

## Naming

This is almost a WeakSet, but we don't bother checking for duplicates, hence "WeakList".

Reviewed By: hoxyq

Differential Revision: D53671483

fbshipit-source-id: 460bfbaa2b8e821281dc352fed0946f99352c9fc

packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp

@@ -33,18 +33,11 @@ std::shared_ptr<InstanceAgent> InstanceTarget::createAgent(
   auto instanceAgent =
       std::make_shared<InstanceAgent>(channel, *this, sessionState);
   instanceAgent->setCurrentRuntime(currentRuntime_.get());
-  agents_.push_back(instanceAgent);
+  agents_.insert(instanceAgent);
   return instanceAgent;
 }
 
-void InstanceTarget::removeExpiredAgents() {
-  // Remove all expired agents.
-  forEachAgent([](auto&) {});
-}
-
 InstanceTarget::~InstanceTarget() {
-  removeExpiredAgents();
-
   // Agents are owned by the session, not by InstanceTarget, but
   // they hold an InstanceTarget& that we must guarantee is valid.
   assert(
@@ -59,7 +52,7 @@ RuntimeTarget& InstanceTarget::registerRuntime(
   currentRuntime_ = RuntimeTarget::create(
       delegate, jsExecutor, makeVoidExecutor(executorFromThis()));
 
-  forEachAgent([currentRuntime = &*currentRuntime_](InstanceAgent& agent) {
+  agents_.forEach([currentRuntime = &*currentRuntime_](InstanceAgent& agent) {
     agent.setCurrentRuntime(currentRuntime);
   });
   return *currentRuntime_;
@@ -69,7 +62,8 @@ void InstanceTarget::unregisterRuntime(RuntimeTarget& Runtime) {
   assert(
       currentRuntime_ && currentRuntime_.get() == &Runtime &&
       "Invalid unregistration");
-  forEachAgent([](InstanceAgent& agent) { agent.setCurrentRuntime(nullptr); });
+  agents_.forEach(
+      [](InstanceAgent& agent) { agent.setCurrentRuntime(nullptr); });
   currentRuntime_.reset();
 }
 

packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h

@@ -10,6 +10,7 @@
 #include "RuntimeTarget.h"
 #include "ScopedExecutor.h"
 #include "SessionState.h"
+#include "WeakList.h"
 
 #include <jsinspector-modern/InspectorInterfaces.h>
 #include <jsinspector-modern/RuntimeAgent.h>
@@ -83,25 +84,7 @@ class InstanceTarget : public EnableExecutorFromThis<InstanceTarget> {
 
   InstanceTargetDelegate& delegate_;
   std::shared_ptr<RuntimeTarget> currentRuntime_{nullptr};
-  std::list<std::weak_ptr<InstanceAgent>> agents_;
-
-  /**
-   * Call the given function for every active agent, and clean up any
-   * references to inactive agents.
-   */
-  template <typename Fn>
-  void forEachAgent(Fn&& fn) {
-    for (auto it = agents_.begin(); it != agents_.end();) {
-      if (auto agent = it->lock()) {
-        fn(*agent);
-        ++it;
-      } else {
-        it = agents_.erase(it);
-      }
-    }
-  }
-
-  void removeExpiredAgents();
+  WeakList<InstanceAgent> agents_;
 };
 
 } // namespace facebook::react::jsinspector_modern

packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp

@@ -115,19 +115,12 @@ std::unique_ptr<ILocalConnection> PageTarget::connect(
   auto session = std::make_shared<PageTargetSession>(
       std::move(connectionToFrontend), controller_, std::move(sessionMetadata));
   session->setCurrentInstance(currentInstance_.get());
-  sessions_.push_back(std::weak_ptr(session));
+  sessions_.insert(std::weak_ptr(session));
   return std::make_unique<CallbackLocalConnection>(
       [session](std::string message) { (*session)(message); });
 }
 
-void PageTarget::removeExpiredSessions() {
-  // Remove all expired sessions.
-  forEachSession([](auto&) {});
-}
-
 PageTarget::~PageTarget() {
-  removeExpiredSessions();
-
   // Sessions are owned by InspectorPackagerConnection, not by PageTarget, but
   // they hold a PageTarget& that we must guarantee is valid.
   assert(
@@ -141,7 +134,7 @@ InstanceTarget& PageTarget::registerInstance(InstanceTargetDelegate& delegate) {
   assert(!currentInstance_ && "Only one instance allowed");
   currentInstance_ =
       InstanceTarget::create(delegate, makeVoidExecutor(executorFromThis()));
-  forEachSession(
+  sessions_.forEach(
       [currentInstance = &*currentInstance_](PageTargetSession& session) {
         session.setCurrentInstance(currentInstance);
       });
@@ -152,7 +145,7 @@ void PageTarget::unregisterInstance(InstanceTarget& instance) {
   assert(
       currentInstance_ && currentInstance_.get() == &instance &&
       "Invalid unregistration");
-  forEachSession(
+  sessions_.forEach(
       [](PageTargetSession& session) { session.setCurrentInstance(nullptr); });
   currentInstance_.reset();
 }

packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h

@@ -8,11 +8,11 @@
 #pragma once
 
 #include "ScopedExecutor.h"
+#include "WeakList.h"
 
 #include <jsinspector-modern/InspectorInterfaces.h>
 #include <jsinspector-modern/InstanceTarget.h>
 
-#include <list>
 #include <optional>
 #include <string>
 
@@ -164,28 +164,10 @@ class JSINSPECTOR_EXPORT PageTarget
   PageTarget(PageTargetDelegate& delegate);
 
   PageTargetDelegate& delegate_;
-  std::list<std::weak_ptr<PageTargetSession>> sessions_;
+  WeakList<PageTargetSession> sessions_;
   PageTargetController controller_{*this};
   std::shared_ptr<InstanceTarget> currentInstance_{nullptr};
 
-  /**
-   * Call the given function for every active session, and clean up any
-   * references to inactive sessions.
-   */
-  template <typename Fn>
-  void forEachSession(Fn&& fn) {
-    for (auto it = sessions_.begin(); it != sessions_.end();) {
-      if (auto session = it->lock()) {
-        fn(*session);
-        ++it;
-      } else {
-        it = sessions_.erase(it);
-      }
-    }
-  }
-
-  void removeExpiredSessions();
-
   inline PageTargetDelegate& getDelegate() {
     return delegate_;
   }

packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp

@@ -32,18 +32,11 @@ std::shared_ptr<RuntimeAgent> RuntimeTarget::createAgent(
       *this,
       sessionState,
       delegate_.createAgentDelegate(channel, sessionState));
-  agents_.push_back(runtimeAgent);
+  agents_.insert(runtimeAgent);
   return runtimeAgent;
 }
 
-void RuntimeTarget::removeExpiredAgents() {
-  // Remove all expired agents.
-  forEachAgent([](auto&) {});
-}
-
 RuntimeTarget::~RuntimeTarget() {
-  removeExpiredAgents();
-
   // Agents are owned by the session, not by RuntimeTarget, but
   // they hold a RuntimeTarget& that we must guarantee is valid.
   assert(

packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h

@@ -12,8 +12,8 @@
 #include "RuntimeAgent.h"
 #include "ScopedExecutor.h"
 #include "SessionState.h"
+#include "WeakList.h"
 
-#include <list>
 #include <memory>
 
 #ifndef JSINSPECTOR_EXPORT
@@ -106,25 +106,7 @@ class JSINSPECTOR_EXPORT RuntimeTarget
 
   RuntimeTargetDelegate& delegate_;
   RuntimeExecutor jsExecutor_;
-  std::list<std::weak_ptr<RuntimeAgent>> agents_;
-
-  /**
-   * Call the given function for every active agent, and clean up any
-   * references to inactive agents.
-   */
-  template <typename Fn>
-  void forEachAgent(Fn&& fn) {
-    for (auto it = agents_.begin(); it != agents_.end();) {
-      if (auto agent = it->lock()) {
-        fn(*agent);
-        ++it;
-      } else {
-        it = agents_.erase(it);
-      }
-    }
-  }
-
-  void removeExpiredAgents();
+  WeakList<RuntimeAgent> agents_;
 };
 
 } // namespace facebook::react::jsinspector_modern

packages/react-native/ReactCommon/jsinspector-modern/WeakList.h

@@ -0,0 +1,79 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <list>
+#include <memory>
+
+namespace facebook::react::jsinspector_modern {
+
+/**
+ * A list that holds weak pointers to objects of type `T`. Null pointers are not
+ * considered to be in the list.
+ *
+ * The list is not thread-safe! The caller is responsible for synchronization.
+ */
+template <typename T>
+class WeakList {
+ public:
+  /**
+   * Call the given function for every element in the list, ensuring the element
+   * is not destroyed for the duration of the call. Elements are visited in the
+   * order they were inserted.
+   *
+   * As a side effect, any null pointers in the underlying list (corresponding
+   * to destroyed elements) will be removed during iteration.
+   */
+  template <typename Fn>
+  void forEach(Fn&& fn) const {
+    for (auto it = ptrs_.begin(); it != ptrs_.end();) {
+      if (auto ptr = it->lock()) {
+        fn(*ptr);
+        ++it;
+      } else {
+        it = ptrs_.erase(it);
+      }
+    }
+  }
+
+  /**
+   * Returns the number of (non-null) elements in the list. The count will only
+   * remain accurate as long as the list is not modified and elements are
+   * not destroyed.
+   *
+   * As a side effect, any null pointers in the underlying list (corresponding
+   * to destroyed elements) will be removed during this method.
+   */
+  size_t size() const {
+    size_t count{0};
+    forEach([&count](const auto&) { ++count; });
+    return count;
+  }
+
+  /**
+   * Returns true if there are no elements in the list.
+   *
+   * As a side effect, any null pointers in the underlying list (corresponding
+   * to destroyed elements) will be removed during this method.
+   */
+  bool empty() const {
+    return !size();
+  }
+
+  /**
+   * Inserts an element into the list.
+   */
+  void insert(std::weak_ptr<T> ptr) {
+    ptrs_.push_back(ptr);
+  }
+
+ private:
+  mutable std::list<std::weak_ptr<T>> ptrs_;
+};
+
+} // namespace facebook::react::jsinspector_modern

packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp

@@ -0,0 +1,91 @@
+/*
+ * 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 <jsinspector-modern/WeakList.h>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include <vector>
+
+using namespace ::testing;
+
+namespace facebook::react::jsinspector_modern {
+
+TEST(WeakListTest, Size) {
+  WeakList<int> list;
+  EXPECT_EQ(list.size(), 0);
+
+  auto p1 = std::make_shared<int>(1);
+  list.insert(p1);
+  EXPECT_EQ(list.size(), 1);
+
+  auto p2 = std::make_shared<int>(2);
+  list.insert(p2);
+  EXPECT_EQ(list.size(), 2);
+
+  p1.reset();
+  EXPECT_EQ(list.size(), 1);
+
+  p2.reset();
+  EXPECT_EQ(list.size(), 0);
+}
+
+TEST(WeakListTest, Empty) {
+  WeakList<int> list;
+  EXPECT_EQ(list.empty(), true);
+
+  auto p1 = std::make_shared<int>(1);
+  list.insert(p1);
+  EXPECT_EQ(list.empty(), false);
+
+  auto p2 = std::make_shared<int>(2);
+  list.insert(p2);
+  EXPECT_EQ(list.empty(), false);
+
+  p1.reset();
+  EXPECT_EQ(list.empty(), false);
+
+  p2.reset();
+  EXPECT_EQ(list.empty(), true);
+}
+
+TEST(WeakListTest, ForEach) {
+  WeakList<int> list;
+  auto p1 = std::make_shared<int>(1);
+  list.insert(p1);
+  auto p2 = std::make_shared<int>(2);
+  list.insert(p2);
+  auto p3 = std::make_shared<int>(3);
+  list.insert(p3);
+
+  p2.reset();
+
+  std::vector<int> visited;
+  list.forEach([&visited](const int& value) { visited.push_back(value); });
+  EXPECT_THAT(visited, ElementsAre(1, 3));
+}
+
+TEST(WeakListTest, ElementsAreAliveDuringCallback) {
+  WeakList<int> list;
+  auto p1 = std::make_shared<int>(1);
+  // A separate weak_ptr to observe the lifetime of `p1`.
+  std::weak_ptr wp1 = p1;
+  list.insert(p1);
+
+  std::vector<int> visited;
+  list.forEach([&](const int& value) {
+    p1.reset();
+    EXPECT_FALSE(wp1.expired());
+    visited.push_back(value);
+  });
+
+  EXPECT_TRUE(wp1.expired());
+  EXPECT_THAT(visited, ElementsAre(1));
+}
+
+} // namespace facebook::react::jsinspector_modern