Skip to content

Commit

Permalink
Update CDPAgent to take unique_ptr for State
Browse files Browse the repository at this point in the history
Summary:
Changing CDPAgent interface to take `unique_ptr` for State. We should
be careful about using `shared_ptr` because it could introduce subtle
lifecycle issues.

The `DomainAgents::initialize()` still requires a `shared_ptr` because
`std::function` is needed to be copyable. `std::move_only_function` is
only C++23, so not available to us yet.

Reviewed By: huntie

Differential Revision: D54437914

fbshipit-source-id: 3d6b54fe0926955e48bc0a03d9d614186699fccf
  • Loading branch information
dannysu authored and facebook-github-bot committed Mar 5, 2024
1 parent f0040cd commit 314395c
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 12 deletions.
17 changes: 9 additions & 8 deletions API/hermes/cdp/CDPAgent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class CDPAgentImpl {
~CDPAgentImpl();

/// Schedule initialization of handlers for each message domain.
void initializeDomainAgents(std::shared_ptr<State> state);
void initializeDomainAgents(std::unique_ptr<State> state);

/// Process a CDP command encoded in \p json.
void handleCommand(std::string json);
Expand Down Expand Up @@ -177,11 +177,12 @@ CDPAgentImpl::~CDPAgentImpl() {
messageCallback_.invalidate();
}

void CDPAgentImpl::initializeDomainAgents(std::shared_ptr<State> state) {
void CDPAgentImpl::initializeDomainAgents(std::unique_ptr<State> state) {
// Call DomainAgents::initialize on the runtime thread.
std::shared_ptr<State> initialState = std::move(state);
runtimeTaskRunner_.enqueueTask(
[domainAgents = domainAgents_, state](HermesRuntime &) {
domainAgents->initialize(state);
[domainAgents = domainAgents_, initialState](HermesRuntime &) {
domainAgents->initialize(initialState);
});
}

Expand Down Expand Up @@ -370,27 +371,27 @@ std::unique_ptr<CDPAgent> CDPAgent::create(
CDPDebugAPI &cdpDebugAPI,
EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
OutboundMessageFunc messageCallback,
std::shared_ptr<State> state) {
std::unique_ptr<State> state) {
return std::unique_ptr<CDPAgent>(new CDPAgent(
executionContextID,
cdpDebugAPI,
enqueueRuntimeTaskCallback,
messageCallback,
state));
std::move(state)));
}

CDPAgent::CDPAgent(
int32_t executionContextID,
CDPDebugAPI &cdpDebugAPI,
EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
OutboundMessageFunc messageCallback,
std::shared_ptr<State> state)
std::unique_ptr<State> state)
: impl_(std::make_unique<CDPAgentImpl>(
executionContextID,
cdpDebugAPI,
enqueueRuntimeTaskCallback,
SynchronizedOutboundCallback(messageCallback))) {
impl_->initializeDomainAgents(state);
impl_->initializeDomainAgents(std::move(state));
}

CDPAgent::~CDPAgent() {}
Expand Down
4 changes: 2 additions & 2 deletions API/hermes/cdp/CDPAgent.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class HERMES_EXPORT CDPAgent {
CDPDebugAPI &cdpDebugAPI,
debugger::EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
OutboundMessageFunc messageCallback,
std::shared_ptr<State> state);
std::unique_ptr<State> state);

public:
/// Create a new CDP Agent. This can be done on an arbitrary thread; the
Expand All @@ -55,7 +55,7 @@ class HERMES_EXPORT CDPAgent {
CDPDebugAPI &cdpDebugAPI,
debugger::EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
OutboundMessageFunc messageCallback,
std::shared_ptr<State> state = nullptr);
std::unique_ptr<State> state = nullptr);

/// Destroy the CDP Agent. This can be done on an arbitrary thread.
/// It's expected that the integrator will continue to process any runtime
Expand Down
4 changes: 2 additions & 2 deletions unittests/API/CDPAgentTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1413,7 +1413,7 @@ TEST_F(CDPAgentTest, DebuggerRestoreState) {
ensureSetBreakpointByUrlResponse(waitForMessage(), msgId++, {});

for (int i = 0; i < 2; i++) {
std::shared_ptr<State> state;
std::unique_ptr<State> state;
if (i == 0) {
// Save CDPAgent state on non-runtime thread and shut everything down.
state = cdpAgent_->getState();
Expand Down Expand Up @@ -1448,7 +1448,7 @@ TEST_F(CDPAgentTest, DebuggerRestoreState) {
*cdpDebugAPI_,
std::bind(&CDPAgentTest::handleRuntimeTask, this, _1),
std::bind(&CDPAgentTest::handleResponse, this, _1),
state);
std::move(state));

sendAndCheckResponse("Debugger.enable", msgId++);
scheduleScript(R"(
Expand Down

0 comments on commit 314395c

Please sign in to comment.