Skip to content

Commit

Permalink
RuntimeTarget refactor - introduce WeakList for Target→Agent refs
Browse files Browse the repository at this point in the history
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
  • Loading branch information
motiz88 authored and facebook-github-bot committed Feb 14, 2024
1 parent d4468fb commit 2661222
Show file tree
Hide file tree
Showing 8 changed files with 184 additions and 87 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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_;
Expand All @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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);
});
Expand All @@ -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();
}
Expand Down
22 changes: 2 additions & 20 deletions packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -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_;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
#include "RuntimeAgent.h"
#include "ScopedExecutor.h"
#include "SessionState.h"
#include "WeakList.h"

#include <list>
#include <memory>

#ifndef JSINSPECTOR_EXPORT
Expand Down Expand Up @@ -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
79 changes: 79 additions & 0 deletions packages/react-native/ReactCommon/jsinspector-modern/WeakList.h
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 2661222

Please sign in to comment.