C++ InspectorPackagerConnection: Reconnect on socket failures (#42158)
Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/42158 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 fbshipit-source-id: 07e5a5c36222dc7bede8bcb17a1f3ced2788736b
This commit is contained in:
Родитель
51b7c681c9
Коммит
8c14997cbd
|
@ -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(
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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;
|
||||
};
|
||||
|
|
|
@ -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]);
|
||||
|
|
Загрузка…
Ссылка в новой задаче