Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

EdmondChuiHW
Copy link
Contributor

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

Hotfix for "Reconnect DevTools" button not working:

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.
  2. Build and run RNTester/FB Wilde
  3. They should be using the local bundled version. App may prompt you to start Metro.
  4. Start Metro
  5. Go to the device Dev Menu (rage shake) and select Reload
  6. 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)
  2. Kill Metro
  3. The RN DevTools should show the "disconnected" dialog
  4. Start Metro
  5. 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.

  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);

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.

PS

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)

Differential Revision: D65952134

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Nov 14, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65952134

Summary:

Changelog:
[iOS][Fixed] - fix `r` & `d` not working from Metro sometimes

While investigating these bugs, I've come across some cases where `r` (Reload) & `d` (Open Dev Menu) not working in Metro.

* 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/)

This is because when we
1. Start app without Metro
1. Start Metro
1. Reload from Dev Menu (rage shake)

`RCTPackagerConnection` did not get notified about the change in bundle URL. It'd stay "listening" to the commands from the local bundle instead of Metro
.

Differential Revision: D65973309
…ok#47623)

Summary:

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.

```
  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);
```

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.

# PS

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)

Differential Revision: D65952134
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65952134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants