C++ InspectorPackagerConnection: Reconnect on socket failures (facebo…

…ok#42158)

Summary:

Changelog: [Internal]

* Ports an existing Java/ObjC InspectorPackagerConnection behaviour to the C++ implementation: socket errors should trigger a reconnection. This was a simple omission in D52134592.
* Clarifies the relationship between the `didFailWithError` and `didClose` methods on `IWebSocketDelegate`: calling either one will terminate the connection (and trigger a reconnection), and it's legal to call `didClose` after `didFailWithError`.
  * I'm also adding logic to ensure we don't double-schedule reconnections if both methods are called.
* Cleans up the scaffolding comments from D52134592

Reviewed By: huntie

Differential Revision: D52576727

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

@@ -25,15 +25,6 @@ static folly::dynamic makePageIdPayload(std::string_view pageId) {
   return folly::dynamic::object("id", pageId);
 }
 
-/*
-TODO(moti): Remove this comment, which is only here to help the diff tool
-understand the relationship between this file and
-RCTInspectorPackagerConnection.m.
-
-@implementation RCTInspectorPackagerConnection
-
-*/
-
 // InspectorPackagerConnection::Impl method definitions
 
 std::shared_ptr<InspectorPackagerConnection::Impl>
@@ -178,6 +169,9 @@ void InspectorPackagerConnection::Impl::didFailWithError(
   if (webSocket_) {
     abort(posixCode, "WebSocket exception", error);
   }
+  if (!closed_ && posixCode != ECONNREFUSED) {
+    reconnect();
+  }
 }
 
 void InspectorPackagerConnection::Impl::didReceiveMessage(
@@ -215,6 +209,9 @@ void InspectorPackagerConnection::Impl::connect() {
 }
 
 void InspectorPackagerConnection::Impl::reconnect() {
+  if (reconnectPending_) {
+    return;
+  }
   if (closed_) {
     LOG(ERROR)
         << "Illegal state: Can't reconnect after having previously been closed.";
@@ -226,10 +223,13 @@ void InspectorPackagerConnection::Impl::reconnect() {
     suppressConnectionErrors_ = true;
   }
 
+  reconnectPending_ = true;
+
   delegate_->scheduleCallback(
       [weakSelf = weak_from_this()] {
         auto strongSelf = weakSelf.lock();
         if (strongSelf && !strongSelf->closed_) {
+          strongSelf->reconnectPending_ = false;
           strongSelf->connect();
         }
       },
@@ -267,18 +267,6 @@ void InspectorPackagerConnection::Impl::disposeWebSocket() {
   webSocket_.reset();
 }
 
-/*
-
-@end
-
-TODO(moti): Remove this comment, which is only here to help the diff tool
-understand the relationship between this file and
-RCTInspectorPackagerConnection.m.
-
-@implementation RCTInspectorRemoteConnection
-
-*/
-
 // InspectorPackagerConnection::RemoteConnectionImpl method definitions
 
 InspectorPackagerConnection::RemoteConnectionImpl::RemoteConnectionImpl(

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

@@ -81,6 +81,9 @@ class InspectorPackagerConnection::Impl
   std::unique_ptr<IWebSocket> webSocket_;
   bool closed_{false};
   bool suppressConnectionErrors_{false};
+
+  // Whether a reconnection is currently pending.
+  bool reconnectPending_{false};
 };
 
 class InspectorPackagerConnection::RemoteConnectionImpl

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

@@ -53,7 +53,8 @@ class IWebSocketDelegate {
   virtual void didReceiveMessage(std::string_view message) = 0;
 
   /**
-   * Called when the socket has been closed.
+   * Called when the socket has been closed. The call is not required if
+   * didFailWithError was called instead.
    */
   virtual void didClose() = 0;
 };

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

@@ -441,6 +441,91 @@ TEST_F(InspectorPackagerConnectionTest, TestConnectThenCloseSocket) {
   EXPECT_FALSE(localConnections_[0]);
 }
 
+TEST_F(InspectorPackagerConnectionTest, TestConnectThenSocketFailure) {
+  // Configure gmock to expect calls in a specific order.
+  InSequence mockCallsMustBeInSequence;
+
+  packagerConnection_->connect();
+  auto pageId = getInspectorInstance().addPage(
+      "mock-title",
+      "mock-vm",
+      localConnections_
+          .lazily_make_unique<std::unique_ptr<IRemoteConnection>>());
+
+  // Connect to the page.
+  webSockets_[0]->getDelegate().didReceiveMessage(sformat(
+      R"({{
+          "event": "connect",
+          "payload": {{
+            "pageId": {0}
+          }}
+        }})",
+      toJson(std::to_string(pageId))));
+  ASSERT_TRUE(localConnections_[0]);
+
+  // Notify that the socket was closed (implicitly, as the result of an error).
+  EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
+  webSockets_[0]->getDelegate().didFailWithError(ECONNABORTED, "Test error");
+  EXPECT_FALSE(localConnections_[0]);
+}
+
+TEST_F(InspectorPackagerConnectionTest, TestExplicitCloseAfterSocketFailure) {
+  // Configure gmock to expect calls in a specific order.
+  InSequence mockCallsMustBeInSequence;
+
+  packagerConnection_->connect();
+  auto pageId = getInspectorInstance().addPage(
+      "mock-title",
+      "mock-vm",
+      localConnections_
+          .lazily_make_unique<std::unique_ptr<IRemoteConnection>>());
+
+  // Connect to the page.
+  webSockets_[0]->getDelegate().didReceiveMessage(sformat(
+      R"({{
+          "event": "connect",
+          "payload": {{
+            "pageId": {0}
+          }}
+        }})",
+      toJson(std::to_string(pageId))));
+  ASSERT_TRUE(localConnections_[0]);
+
+  // Notify that the socket was closed (implicitly, as the result of an error).
+  EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
+
+  // For convenience, the mock scheduleCallback usually calls the callback
+  // immediately. However, here we want the async behaviour so we can ensure
+  // only one reconnection gets scheduled.
+  std::function<void()> scheduledCallback;
+  EXPECT_CALL(*packagerConnectionDelegate(), scheduleCallback(_, _))
+      .WillOnce(
+          [&](std::function<void(void)> callback, std::chrono::milliseconds) {
+            EXPECT_FALSE(scheduledCallback);
+            scheduledCallback = callback;
+          })
+      .RetiresOnSaturation();
+
+  {
+    // The WebSocket instance gets destroyed during didFailWithError, so extract
+    // the delegate in order to call didClose.
+    std::shared_ptr webSocketDelegate = webSockets_[0]->delegate.lock();
+
+    webSocketDelegate->didFailWithError(ECONNABORTED, "Test error");
+    webSocketDelegate->didClose();
+  }
+
+  ASSERT_FALSE(localConnections_[0]);
+  // We're still disconnected since we haven't called the reconnect callback.
+  ASSERT_FALSE(packagerConnection_->isConnected());
+
+  // Flush the callback queue.
+  EXPECT_TRUE(scheduledCallback);
+  scheduledCallback();
+
+  ASSERT_TRUE(packagerConnection_->isConnected());
+}
+
 TEST_F(
     InspectorPackagerConnectionTest,
     TestConnectWhileAlreadyConnectedCausesDisconnection) {
@@ -643,6 +728,53 @@ TEST_F(InspectorPackagerConnectionTest, TestReconnectFailure) {
   EXPECT_FALSE(packagerConnection_->isConnected());
 }
 
+TEST_F(InspectorPackagerConnectionTest, TestReconnectOnSocketError) {
+  // Configure gmock to expect calls in a specific order.
+  InSequence mockCallsMustBeInSequence;
+
+  packagerConnection_->connect();
+  ASSERT_TRUE(webSockets_[0]);
+  EXPECT_CALL(*packagerConnectionDelegate(), scheduleCallback(_, _))
+      .RetiresOnSaturation();
+  webSockets_[0]->getDelegate().didFailWithError(ECONNRESET, "Test error");
+  EXPECT_FALSE(webSockets_[0]);
+  EXPECT_TRUE(webSockets_[1]);
+  EXPECT_TRUE(packagerConnection_->isConnected());
+
+  // Stops attempting to reconnect after closeQuietly
+
+  packagerConnection_->closeQuietly();
+}
+
+TEST_F(InspectorPackagerConnectionTest, TestReconnectOnSocketErrorWithNoCode) {
+  // Configure gmock to expect calls in a specific order.
+  InSequence mockCallsMustBeInSequence;
+
+  packagerConnection_->connect();
+  ASSERT_TRUE(webSockets_[0]);
+  EXPECT_CALL(*packagerConnectionDelegate(), scheduleCallback(_, _))
+      .RetiresOnSaturation();
+  webSockets_[0]->getDelegate().didFailWithError(std::nullopt, "Test error");
+  EXPECT_FALSE(webSockets_[0]);
+  EXPECT_TRUE(webSockets_[1]);
+  EXPECT_TRUE(packagerConnection_->isConnected());
+
+  // Stops attempting to reconnect after closeQuietly
+
+  packagerConnection_->closeQuietly();
+}
+
+TEST_F(InspectorPackagerConnectionTest, TestNoReconnectOnConnectionRefused) {
+  // Configure gmock to expect calls in a specific order.
+  InSequence mockCallsMustBeInSequence;
+
+  packagerConnection_->connect();
+  ASSERT_TRUE(webSockets_[0]);
+  webSockets_[0]->getDelegate().didFailWithError(ECONNREFUSED, "Test error");
+  EXPECT_FALSE(webSockets_[0]);
+  EXPECT_FALSE(packagerConnection_->isConnected());
+}
+
 TEST_F(InspectorPackagerConnectionTest, TestUnknownEvent) {
   packagerConnection_->connect();
   ASSERT_TRUE(webSockets_[0]);