Fix bridgeless triggering reloads twice from BridgelessDevSupportMana…

…ger (facebook#44554) Summary: Pull Request resolved: facebook#44554 Noticed than when reload is triggered by Metro (`handleReloadJS`), the application would often get stuck and not respond to further reload commands. Often an IOException would get printed as well, due to concurrent bundle loads happening. Changelog: [Android][Fixed] Improved resiliency of reloads when bundle loading fails Reviewed By: RSNara Differential Revision: D57112152 fbshipit-source-id: b0bf8c8311264504684a137c0910e2eeb008b0c7
rshest · May 15, 2024 · 524e3ee · 524e3ee
1 parent d0bb396
commit 524e3ee
Show file tree

Hide file tree

Showing 9 changed files with 43 additions and 52 deletions.
diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api
@@ -2115,7 +2115,6 @@ public abstract class com/facebook/react/devsupport/DevSupportManagerBase : com/
 	public fun openDebugger ()V
 	public fun processErrorCustomizers (Landroid/util/Pair;)Landroid/util/Pair;
 	public fun registerErrorCustomizer (Lcom/facebook/react/devsupport/interfaces/ErrorCustomizer;)V
-	public fun reloadJSFromServer (Ljava/lang/String;)V
 	public fun reloadJSFromServer (Ljava/lang/String;Lcom/facebook/react/devsupport/interfaces/BundleLoadCallback;)V
 	public fun reloadSettings ()V
 	public fun setDevSupportEnabled (Z)V
@@ -2282,7 +2281,6 @@ public class com/facebook/react/devsupport/ReleaseDevSupportManager : com/facebo
 	public fun openDebugger ()V
 	public fun processErrorCustomizers (Landroid/util/Pair;)Landroid/util/Pair;
 	public fun registerErrorCustomizer (Lcom/facebook/react/devsupport/interfaces/ErrorCustomizer;)V
-	public fun reloadJSFromServer (Ljava/lang/String;)V
 	public fun reloadJSFromServer (Ljava/lang/String;Lcom/facebook/react/devsupport/interfaces/BundleLoadCallback;)V
 	public fun reloadSettings ()V
 	public fun setDevSupportEnabled (Z)V
@@ -2341,6 +2339,7 @@ public class com/facebook/react/devsupport/WebsocketJavaScriptExecutor$Websocket
 }
 
 public abstract interface class com/facebook/react/devsupport/interfaces/BundleLoadCallback {
+	public fun onError (Ljava/lang/Exception;)V
 	public abstract fun onSuccess ()V
 }
 
@@ -2394,7 +2393,6 @@ public abstract interface class com/facebook/react/devsupport/interfaces/DevSupp
 	public abstract fun openDebugger ()V
 	public abstract fun processErrorCustomizers (Landroid/util/Pair;)Landroid/util/Pair;
 	public abstract fun registerErrorCustomizer (Lcom/facebook/react/devsupport/interfaces/ErrorCustomizer;)V
-	public abstract fun reloadJSFromServer (Ljava/lang/String;)V
 	public abstract fun reloadJSFromServer (Ljava/lang/String;Lcom/facebook/react/devsupport/interfaces/BundleLoadCallback;)V
 	public abstract fun reloadSettings ()V
 	public abstract fun setDevSupportEnabled (Z)V

diff --git a/...ive/ReactAndroid/src/main/java/com/facebook/react/devsupport/BridgeDevSupportManager.java b/...ive/ReactAndroid/src/main/java/com/facebook/react/devsupport/BridgeDevSupportManager.java
@@ -62,8 +62,6 @@
  */
 public final class BridgeDevSupportManager extends DevSupportManagerBase {
   private boolean mIsSamplingProfilerEnabled = false;
-  private ReactInstanceDevHelper mReactInstanceManagerHelper;
-  private @Nullable DevLoadingViewManager mDevLoadingViewManager;
 
   public BridgeDevSupportManager(
       Context applicationContext,
@@ -211,7 +209,11 @@ public void handleReloadJS() {
       String bundleURL =
           getDevServerHelper()
               .getDevServerBundleURL(Assertions.assertNotNull(getJSAppBundleName()));
-      reloadJSFromServer(bundleURL);
+      reloadJSFromServer(
+          bundleURL,
+          () ->
+              UiThreadUtil.runOnUiThread(
+                  () -> getReactInstanceDevHelper().onJSBundleLoadedFromServer()));
     }
   }
 

diff --git a/...act-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/BundleDownloader.java b/...act-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/BundleDownloader.java
@@ -24,6 +24,7 @@
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.Response;
+import okhttp3.ResponseBody;
 import okio.Buffer;
 import okio.BufferedSource;
 import okio.Okio;
@@ -146,14 +147,16 @@ public void onResponse(Call call, final Response response) throws IOException {
               } else {
                 // In case the server doesn't support multipart/mixed responses, fallback to normal
                 // download.
-                processBundleResult(
-                    url,
-                    r.code(),
-                    r.headers(),
-                    Okio.buffer(r.body().source()),
-                    outputFile,
-                    bundleInfo,
-                    callback);
+                try (ResponseBody body = r.body()) {
+                  processBundleResult(
+                      url,
+                      r.code(),
+                      r.headers(),
+                      r.body().source(),
+                      outputFile,
+                      bundleInfo,
+                      callback);
+                }
               }
             }
           }

diff --git a/...ative/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.java b/...ative/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.java
@@ -910,12 +910,6 @@ private void updateLastErrorInfo(
     mLastErrorType = errorType;
   }
 
-  public void reloadJSFromServer(final String bundleURL) {
-    reloadJSFromServer(
-        bundleURL,
-        () -> UiThreadUtil.runOnUiThread(mReactInstanceDevHelper::onJSBundleLoadedFromServer));
-  }
-
   public void reloadJSFromServer(final String bundleURL, final BundleLoadCallback callback) {
     ReactMarker.logMarker(ReactMarkerConstants.DOWNLOAD_START);
 
@@ -954,6 +948,7 @@ public void onFailure(final Exception cause) {
             }
             FLog.e(ReactConstants.TAG, "Unable to download JS bundle", cause);
             reportBundleLoadingFailure(cause);
+            callback.onError(cause);
           }
         },
         mJSBundleDownloadedFile,

diff --git a/...ve/ReactAndroid/src/main/java/com/facebook/react/devsupport/ReleaseDevSupportManager.java b/...ve/ReactAndroid/src/main/java/com/facebook/react/devsupport/ReleaseDevSupportManager.java
@@ -138,9 +138,6 @@ public void reloadSettings() {}
   @Override
   public void handleReloadJS() {}
 
-  @Override
-  public void reloadJSFromServer(String bundleURL) {}
-
   @Override
   public void reloadJSFromServer(final String bundleURL, final BundleLoadCallback callback) {}
 

diff --git a/...ReactAndroid/src/main/java/com/facebook/react/devsupport/interfaces/BundleLoadCallback.kt b/...ReactAndroid/src/main/java/com/facebook/react/devsupport/interfaces/BundleLoadCallback.kt
@@ -8,5 +8,7 @@
 package com.facebook.react.devsupport.interfaces
 
 public fun interface BundleLoadCallback {
-  public fun onSuccess()
+  public fun onSuccess(): Unit
+
+  public fun onError(cause: Exception): Unit = Unit
 }
diff --git a/.../ReactAndroid/src/main/java/com/facebook/react/devsupport/interfaces/DevSupportManager.kt b/.../ReactAndroid/src/main/java/com/facebook/react/devsupport/interfaces/DevSupportManager.kt
@@ -58,18 +58,16 @@ public interface DevSupportManager : JSExceptionHandler {
 
   public fun stopInspector()
 
-  public fun onNewReactContextCreated(reactContext: ReactContext?)
+  public fun onNewReactContextCreated(reactContext: ReactContext)
 
-  public fun onReactInstanceDestroyed(reactContext: ReactContext?)
+  public fun onReactInstanceDestroyed(reactContext: ReactContext)
 
   public fun hasUpToDateJSBundleInCache(): Boolean
 
   public fun reloadSettings()
 
   public fun handleReloadJS()
 
-  public fun reloadJSFromServer(bundleURL: String)
-
   public fun reloadJSFromServer(bundleURL: String, callback: BundleLoadCallback)
 
   public fun loadSplitBundleFromServer(bundlePath: String, callback: DevSplitBundleCallback)

diff --git a/...ve/ReactAndroid/src/main/java/com/facebook/react/runtime/BridgelessDevSupportManager.java b/...ve/ReactAndroid/src/main/java/com/facebook/react/runtime/BridgelessDevSupportManager.java
@@ -11,9 +11,7 @@
 import android.content.Context;
 import android.os.Bundle;
 import android.view.View;
-import com.facebook.debug.holder.PrinterHolder;
-import com.facebook.debug.tags.ReactDebugOverlayTags;
-import com.facebook.infer.annotation.Assertions;
+import androidx.annotation.Nullable;
 import com.facebook.infer.annotation.Nullsafe;
 import com.facebook.react.bridge.JSBundleLoader;
 import com.facebook.react.bridge.JavaJSExecutor;
@@ -27,7 +25,6 @@
 import com.facebook.react.modules.core.DeviceEventManagerModule;
 import com.facebook.react.runtime.internal.bolts.Continuation;
 import com.facebook.react.runtime.internal.bolts.Task;
-import javax.annotation.Nullable;
 
 /**
  * An implementation of {@link com.facebook.react.devsupport.interfaces.DevSupportManager} that
@@ -103,24 +100,18 @@ public void handleReloadJS() {
     // dismiss redbox if exists
     hideRedboxDialog();
     mReactHost.reload("BridgelessDevSupportManager.handleReloadJS()");
-
-    PrinterHolder.getPrinter()
-        .logMessage(ReactDebugOverlayTags.RN_CORE, "RNCore: load from Server");
-    String bundleURL =
-        getDevServerHelper().getDevServerBundleURL(Assertions.assertNotNull(getJSAppBundleName()));
-    reloadJSFromServer(bundleURL);
   }
 
   private static ReactInstanceDevHelper createInstanceDevHelper(final ReactHostImpl reactHost) {
     return new ReactInstanceDevHelper() {
       @Override
       public void onReloadWithJSDebugger(JavaJSExecutor.Factory proxyExecutorFactory) {
-        // Not implemented
+        // Not implemented, only used by BridgeDevSupportManager to reload with proxy executor
       }
 
       @Override
       public void onJSBundleLoadedFromServer() {
-        // Not implemented
+        // Not implemented, only referenced by BridgeDevSupportManager
       }
 
       @Override
@@ -133,7 +124,7 @@ public void toggleElementInspector() {
         }
       }
 
-      @androidx.annotation.Nullable
+      @Nullable
       @Override
       public Activity getCurrentActivity() {
         return reactHost.getLastUsedActivity();
@@ -144,7 +135,7 @@ public JavaScriptExecutorFactory getJavaScriptExecutorFactory() {
         throw new IllegalStateException("Not implemented for bridgeless mode");
       }
 
-      @androidx.annotation.Nullable
+      @Nullable
       @Override
       public View createRootView(String appKey) {
         Activity currentActivity = getCurrentActivity();

diff --git a/...ges/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java b/...ges/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java
@@ -52,6 +52,7 @@
 import com.facebook.react.devsupport.DevSupportManagerBase;
 import com.facebook.react.devsupport.InspectorFlags;
 import com.facebook.react.devsupport.ReleaseDevSupportManager;
+import com.facebook.react.devsupport.interfaces.BundleLoadCallback;
 import com.facebook.react.devsupport.interfaces.DevSupportManager;
 import com.facebook.react.devsupport.interfaces.DevSupportManager.PausedInDebuggerOverlayCommandListener;
 import com.facebook.react.fabric.ComponentFactory;
@@ -1183,10 +1184,6 @@ private Task<Boolean> isMetroRunning() {
     return taskCompletionSource.getTask();
   }
 
-  /**
-   * TODO(T104078367): Ensure that if creating this JSBundleLoader fails, we route the errors
-   * through ReactHost's error reporting pipeline
-   */
   private Task<JSBundleLoader> loadJSBundleFromMetro() {
     final String method = "loadJSBundleFromMetro()";
     log(method);
@@ -1202,12 +1199,20 @@ private Task<JSBundleLoader> loadJSBundleFromMetro() {
 
     asyncDevSupportManager.reloadJSFromServer(
         bundleURL,
-        () -> {
-          log(method, "Creating BundleLoader");
-          JSBundleLoader bundleLoader =
-              JSBundleLoader.createCachedBundleFromNetworkLoader(
-                  bundleURL, asyncDevSupportManager.getDownloadedJSBundleFile());
-          taskCompletionSource.setResult(bundleLoader);
+        new BundleLoadCallback() {
+          @Override
+          public void onSuccess() {
+            log(method, "Creating BundleLoader");
+            JSBundleLoader bundleLoader =
+                JSBundleLoader.createCachedBundleFromNetworkLoader(
+                    bundleURL, asyncDevSupportManager.getDownloadedJSBundleFile());
+            taskCompletionSource.setResult(bundleLoader);
+          }
+
+          @Override
+          public void onError(Exception cause) {
+            taskCompletionSource.setError(cause);
+          }
         });
 
     return taskCompletionSource.getTask();