fix /open-debugger failing after relaunching Metro/target app (#47623)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47623

Changelog:
[iOS][Fixed] - "Reconnect DevTools" button not working sometimes

Hotfix for "Reconnect DevTools" button not working:

* T206141946 / [WP: Reconnecting dev tools does not work after restarting the app](https://fb.workplace.com/groups/rn.debugger.feedback/posts/1107620434125533)
* T206754760 / [WP: Can't launch DevTools from Metro sometimes](https://fb.workplace.com/groups/rn.debugger.feedback/posts/1112235073664069/)

Basically, this dialog will keep reappearing like a bad dream:

 {F1960030622}

# Repro

Part 1 (Fixed in base stack D65973309)
1. Do NOT have Metro ready.
1. Build and run RNTester/FB Wilde
1. They should be using the local bundled version. App may prompt you to start Metro.
1. Start Metro
1. Go to the device Dev Menu (rage shake) and select Reload
1. Press `r` or `d` in Metro

Expected: Reload and Dev Menu work accordingly
Actual: Metro fails with `No apps connected`:

 {F1960039703}

Part 2 (Fixed in this diff)
1. Open React Native DevTools via Metro `j` key or Dev Menu (rage shake)
1. Kill Metro
1. The RN DevTools should show the "disconnected" dialog
1. Start Metro
1. Click "Reconnect DevTools" in RN DevTools

Expected: reconnects
Actual: dialog reappears with an error in Metro:
{F1960043097}

Interestingly, the `r` and `d` keys from Metro works.

# Root cause(s)
Part 1: See D65973309
Part 2:
The error indicates the target/device failed to call `/inspector/device` to register itself. The subsequent calls to `/json/list` returns empty and `/open-debugger` throws.

1. But because `r` & `d` (heh) works, we can observe that there is some kind of auto-reconnect mechanism:

https://www.internalfb.com/code/fbsource/[cfe1706a60b2]/xplat/js/react-native-github/packages/react-native/Libraries/WebSocket/RCTReconnectingWebSocket.m?lines=77-82

1. We do have auto-reconnect for `j` too:

https://www.internalfb.com/code/fbsource/[cfe1706a60b2]/xplat/js/react-native-github/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp?lines=246-254

But unfortunately it only tries once. A long-term fix would be calling reconnect recursively like the Objective-C impl above, e.g.

Edit: the long-term fix ! See V3

```
  delegate_->scheduleCallback(
      [weakSelf = weak_from_this()] {
        auto strongSelf = weakSelf.lock();
        if (strongSelf && !strongSelf->closed_) {
          strongSelf->reconnectPending_ = false;
          strongSelf->connect();

          // Keep trying. Never. Give. Up.
          if (!strongSelf->isConnected()) {
            strongSelf->reconnect();
          }
        }
      },
      RECONNECT_DELAY);
```

Edit: I snuck in some time during React Native London Conf and got the long-term fix going! ✌️

{gif:7iyrns4l}

~~It appears that the current impl of `isConnected()` is not a true reflection of the web socket state. My time box for this task ran out, so we'll do a hot fix for the short-term: since we know `r` & `d` reliably reconnects, we'll piggy-back on its lifecycle to attempt reconnection. This works. I'm going on PTO for the year, so the follow-up task is up for grabs here: T207775935.~~

# PS

1. If you start the app with Metro running in step 1, this bug is not present. This is the reason why FB Wilde/Marketplace/Quantum engineers run into this more often (because its custom menu changes the JS URL after start up)
2. This auto-reconnect does not mean the RN DevTools frontend will dismiss the dialog automatically. This is only about the `Metro <> Device` in `Frontend <> Metro <> Device`.
    1. Current impl in Metro/Inspector Proxy means that whenever the `Metro <> Device` connection is terminated, the `Frontend <> Metro` connection is killed.
    1. This diff helps restore the `Metro <> Device` connection.
    1. The `Frontend <> Metro` connection does not currently get reconnected. We can make the Frontend do so, with consideration of preserving logs/states, etc.

Reviewed By: robhogan

Differential Revision: D65952134

fbshipit-source-id: 3abd16f6a7b7ed50e8acdb7c753cc4fbd3317236
This commit is contained in:
Edmond Chui 2024-11-18 11:30:23 -08:00 committed by Facebook GitHub Bot
parent 3986eefed1
commit 8507204b53
4 changed files with 19 additions and 12 deletions

View File

@ -32,6 +32,9 @@ std::unique_ptr<IWebSocket> RCTCxxInspectorPackagerConnectionDelegate::connectWe
std::weak_ptr<IWebSocketDelegate> delegate)
{
auto *adapter = [[RCTCxxInspectorWebSocketAdapter alloc] initWithURL:url delegate:delegate];
if (!adapter) {
return nullptr;
}
return std::make_unique<WebSocket>(adapter);
}

View File

@ -19,6 +19,8 @@
using namespace facebook::react::jsinspector_modern;
static const uint64_t MAX_CONNECTING_TIME_NS = 2 * 1000000000L;
namespace {
NSString *NSStringFromUTF8StringView(std::string_view view)
{
@ -39,6 +41,15 @@ NSString *NSStringFromUTF8StringView(std::string_view view)
_webSocket = [[SRWebSocket alloc] initWithURL:[NSURL URLWithString:NSStringFromUTF8StringView(url)]];
_webSocket.delegate = self;
[_webSocket open];
uint64_t startTime = clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW);
while ([_webSocket readyState] == SR_CONNECTING) {
if ((clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW) - startTime) > MAX_CONNECTING_TIME_NS) {
break;
}
}
if ([_webSocket readyState] != SR_OPEN) {
return nil;
}
}
return self;
}

View File

@ -180,7 +180,7 @@ void InspectorPackagerConnection::Impl::didFailWithError(
if (webSocket_) {
abort(posixCode, "WebSocket exception", error);
}
if (!closed_ && posixCode != ECONNREFUSED) {
if (!closed_) {
reconnect();
}
}
@ -249,6 +249,10 @@ void InspectorPackagerConnection::Impl::reconnect() {
if (strongSelf && !strongSelf->closed_) {
strongSelf->reconnectPending_ = false;
strongSelf->connect();
if (!strongSelf->isConnected()) {
strongSelf->reconnect();
}
}
},
RECONNECT_DELAY);

View File

@ -846,17 +846,6 @@ TEST_F(InspectorPackagerConnectionTest, TestReconnectOnSocketErrorWithNoCode) {
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]);