CDPAgent State as value type

Summary: Refactors CDPAgent's state persistence API to treat `State` as a **move-only, default-constructible** value type that represents a handle to a piece of private state. This simplifies the API by removing a layer of indirection (the outer `unique_ptr<State>`).

Reviewed By: mattbfb

Differential Revision: D54580878

fbshipit-source-id: 6c7ea0cea6cfd0b484163eba51162944c3cd5c6f

API/hermes/cdp/CDPAgent.cpp

@@ -32,9 +32,15 @@ struct State::Private {
   std::unique_ptr<DebuggerDomainState> debuggerDomainState;
 };
 
+State::State() : privateState_(nullptr) {}
+
 State::State(std::unique_ptr<Private> privateState)
     : privateState_(std::move(privateState)) {}
 
+State::State(State &&other) noexcept = default;
+
+State &State::operator=(State &&other) noexcept = default;
+
 State::~State() = default;
 
 /// Implementation of the CDP Agent. This class accepts CDP commands from
@@ -52,7 +58,7 @@ class CDPAgentImpl {
   ~CDPAgentImpl();
 
   /// Schedule initialization of handlers for each message domain.
-  void initializeDomainAgents(std::unique_ptr<State> state);
+  void initializeDomainAgents(State state);
 
   /// Process a CDP command encoded in \p json.
   void handleCommand(std::string json);
@@ -62,7 +68,7 @@ class CDPAgentImpl {
   void enableRuntimeDomain();
 
   /// Extract state to be persisted across reloads.
-  std::unique_ptr<State> getState();
+  State getState();
 
  private:
   /// Collection of domain-specific message handlers. These handlers require
@@ -77,7 +83,7 @@ class CDPAgentImpl {
         SynchronizedOutboundCallback messageCallback);
 
     /// Create the domain handlers and subscribing to any external events.
-    void initialize(std::shared_ptr<State> state);
+    void initialize(State state);
 
     /// Releasing any domain handlers and event subscriptions.
     void dispose();
@@ -176,12 +182,16 @@ CDPAgentImpl::~CDPAgentImpl() {
   messageCallback_.invalidate();
 }
 
-void CDPAgentImpl::initializeDomainAgents(std::unique_ptr<State> state) {
+void CDPAgentImpl::initializeDomainAgents(State state) {
   // Call DomainAgents::initialize on the runtime thread.
-  std::shared_ptr<State> initialState = std::move(state);
+  // NOTE: std::function is copyable but State isn't, so we need to pass it
+  // through a shared_ptr.
+  std::shared_ptr<State> stateHolder =
+      std::make_shared<State>(std::move(state));
   runtimeTaskRunner_.enqueueTask(
-      [domainAgents = domainAgents_, initialState](HermesRuntime &) {
-        domainAgents->initialize(initialState);
+      [domainAgents = domainAgents_,
+       stateHolder = std::move(stateHolder)](HermesRuntime &) {
+        domainAgents->initialize(std::move(*stateHolder));
       });
 }
 
@@ -209,14 +219,14 @@ void CDPAgentImpl::enableRuntimeDomain() {
       });
 }
 
-std::unique_ptr<State> CDPAgentImpl::getState() {
+State CDPAgentImpl::getState() {
   // This function might not be called on the runtime thread. Functions on
   // DomainAgents expect to be called on the runtime thread because they
   // manipulate the runtime. In this case, it's still fine to call
   // getDebuggerDomainState() because internally it's protected by a mutex so no
   // DomainAgents functions can simultaneously manipulate the runtime and get
   // the state.
-  return std::make_unique<State>(std::make_unique<State::Private>(
+  return State(std::make_unique<State::Private>(
       domainAgents_->getDebuggerDomainState()));
 }
 
@@ -232,11 +242,11 @@ CDPAgentImpl::DomainAgents::DomainAgents(
       messageCallback_(std::move(messageCallback)),
       objTable_(std::make_shared<RemoteObjectsTable>()) {}
 
-void CDPAgentImpl::DomainAgents::initialize(std::shared_ptr<State> state) {
+void CDPAgentImpl::DomainAgents::initialize(State state) {
   std::lock_guard<std::mutex> lock(mutex_);
   std::unique_ptr<DebuggerDomainState> debuggerState = nullptr;
   if (state) {
-    debuggerState = std::move(state->get().debuggerDomainState);
+    debuggerState = std::move(state->debuggerDomainState);
   }
   debuggerAgent_ = std::make_unique<DebuggerDomainAgent>(
       executionContextID_,
@@ -368,7 +378,7 @@ std::unique_ptr<CDPAgent> CDPAgent::create(
     CDPDebugAPI &cdpDebugAPI,
     EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
     OutboundMessageFunc messageCallback,
-    std::unique_ptr<State> state) {
+    State state) {
   return std::unique_ptr<CDPAgent>(new CDPAgent(
       executionContextID,
       cdpDebugAPI,
@@ -382,7 +392,7 @@ CDPAgent::CDPAgent(
     CDPDebugAPI &cdpDebugAPI,
     EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
     OutboundMessageFunc messageCallback,
-    std::unique_ptr<State> state)
+    State state)
     : impl_(std::make_unique<CDPAgentImpl>(
           executionContextID,
           cdpDebugAPI,
@@ -401,7 +411,7 @@ void CDPAgent::enableRuntimeDomain() {
   impl_->enableRuntimeDomain();
 }
 
-std::unique_ptr<State> CDPAgent::getState() {
+State CDPAgent::getState() {
   return impl_->getState();
 }
 

API/hermes/cdp/CDPAgent.h

@@ -23,7 +23,42 @@ using OutboundMessageFunc = std::function<void(const std::string &)>;
 
 class CDPAgentImpl;
 class CDPDebugAPI;
-struct State;
+
+/// Public-facing wrapper for internal CDP state that can be preserved across
+/// reloads.
+struct HERMES_EXPORT State {
+  /// Incomplete type that stores the actual state.
+  struct Private;
+
+  /// Create a new empty wrapper.
+  State();
+  /// Create a new wrapper with the provided \p privateState.
+  explicit State(std::unique_ptr<Private> privateState);
+
+  State(const State &other) = delete;
+  State &operator=(const State &other) = delete;
+  State(State &&other) noexcept;
+  State &operator=(State &&other) noexcept;
+  ~State();
+
+  inline operator bool() const {
+    return privateState_ != nullptr;
+  }
+
+  /// Get the wrapped state.
+  inline Private &operator*() {
+    return *privateState_.get();
+  }
+
+  /// Get the wrapped state.
+  inline Private *operator->() {
+    return privateState_.get();
+  }
+
+ private:
+  /// Pointer to the actual stored state, hidden from users of this wrapper.
+  std::unique_ptr<Private> privateState_;
+};
 
 /// An agent for interacting with the provided \p runtime and
 /// \p asyncDebuggerAPI via CDP messages in the Debugger, Runtime, Profiler,
@@ -45,7 +80,7 @@ class HERMES_EXPORT CDPAgent {
       CDPDebugAPI &cdpDebugAPI,
       debugger::EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
       OutboundMessageFunc messageCallback,
-      std::unique_ptr<State> state);
+      State state);
 
  public:
   /// Create a new CDP Agent. This can be done on an arbitrary thread; the
@@ -55,7 +90,7 @@ class HERMES_EXPORT CDPAgent {
       CDPDebugAPI &cdpDebugAPI,
       debugger::EnqueueRuntimeTaskFunc enqueueRuntimeTaskCallback,
       OutboundMessageFunc messageCallback,
-      std::unique_ptr<State> state = nullptr);
+      State state = {});
 
   /// Destroy the CDP Agent. This can be done on an arbitrary thread.
   /// It's expected that the integrator will continue to process any runtime
@@ -72,34 +107,14 @@ class HERMES_EXPORT CDPAgent {
 
   /// Extract state to be persisted across reloads. This can be called from
   /// arbitrary threads.
-  std::unique_ptr<State> getState();
+  State getState();
 
  private:
   /// This should be a unique_ptr to provide predictable destruction time lined
   /// up with when CDPAgent is destroyed. Do not use shared_ptr.
   std::unique_ptr<CDPAgentImpl> impl_;
 };
 
-/// Public-facing wrapper for internal CDP state that can be preserved across
-/// reloads.
-struct HERMES_EXPORT State {
-  /// Incomplete type that stores the actual state.
-  struct Private;
-
-  /// Create a new wrapper with the provided \p privateState.
-  explicit State(std::unique_ptr<Private> privateState);
-  ~State();
-
-  /// Get the wrapped state.
-  Private &get() {
-    return *privateState_.get();
-  }
-
- private:
-  /// Pointer to the actual stored state, hidden from users of this wrapper.
-  std::unique_ptr<Private> privateState_;
-};
-
 } // namespace cdp
 } // namespace hermes
 } // namespace facebook

unittests/API/CDPAgentTest.cpp

@@ -1412,7 +1412,7 @@ TEST_F(CDPAgentTest, DebuggerRestoreState) {
   ensureSetBreakpointByUrlResponse(waitForMessage(), msgId++, {});
 
   for (int i = 0; i < 2; i++) {
-    std::unique_ptr<State> state;
+    State state;
     if (i == 0) {
       // Save CDPAgent state on non-runtime thread and shut everything down.
       state = cdpAgent_->getState();