diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp index 536df0910f..7aa01dd97e 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp +++ b/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 @@ -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( diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h index 3a2363908a..704e3f1996 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h @@ -81,6 +81,9 @@ class InspectorPackagerConnection::Impl std::unique_ptr webSocket_; bool closed_{false}; bool suppressConnectionErrors_{false}; + + // Whether a reconnection is currently pending. + bool reconnectPending_{false}; }; class InspectorPackagerConnection::RemoteConnectionImpl diff --git a/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h b/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h index 06cabca5a0..a9606127d3 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h +++ b/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; }; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp index decbc17b3a..7dd0ebccf1 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp +++ b/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>()); + + // 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>()); + + // 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 scheduledCallback; + EXPECT_CALL(*packagerConnectionDelegate(), scheduleCallback(_, _)) + .WillOnce( + [&](std::function 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]);