From f4372240429658b4050090b1f3a384cb6d440fe4 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 21 Aug 2023 12:43:42 -0700 Subject: [PATCH] Enhance/fix error reporting in reload and destroy Summary: The new reload/create/destroy methods work by chaining tasks together. This task chain has the type Task. **The problem:** If any step in the chain fails, task.getResult() actually returns null - not the ReactInstance. Many steps in the existing reload() and destroy() task chains don't account for this case. So: - The reload() and destroy() task chains sometimes swallow errors. - Sometimes steps in the reload() and destroy() task chains don't execute: they use .successTask This diff makes two changes: 1. Ensure each step **always** executes (i.e: use .continueWith vs .success) 2. Ensure each step first checks if the Task isn't faulted/cancelled. If the task is faulted/cancelled, a soft exception gets reported, and the current ReactInstance gets returned. Changelog: [Internal Reviewed By: mdvacca Differential Revision: D48080779 fbshipit-source-id: 22f03ef1a54b538d01eeb5ecde6d82a84d32f1f8 --- .../react/bridgeless/ReactHostImpl.java | 186 +++++++++++++----- .../react/bridgeless/internal/bolts/Task.java | 3 +- 2 files changed, 134 insertions(+), 55 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java index b59ffafeffcd23..4ca4441297a2ae 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java @@ -1180,6 +1180,61 @@ private void startAttachedSurfaces(String method, ReactInstance reactInstance) { @ThreadConfined("ReactHost") private @Nullable Task mReloadTask = null; + private interface ReactInstanceTaskUnwrapper { + @Nullable + ReactInstance unwrap(Task task, String stage); + } + + private ReactInstanceTaskUnwrapper createReactInstanceUnwraper( + String tag, String method, String reason) { + + return (task, stage) -> { + final ReactInstance reactInstance = task.getResult(); + final ReactInstance currentReactInstance = mReactInstanceTaskRef.get().getResult(); + + final String stageLabel = "Stage: " + stage; + final String reasonLabel = tag + " reason: " + reason; + if (task.isFaulted()) { + final Exception ex = task.getError(); + final String faultLabel = "Fault reason: " + ex.getMessage(); + raiseSoftException( + method, + tag + + ": ReactInstance task faulted. " + + stageLabel + + ". " + + faultLabel + + ". " + + reasonLabel); + return currentReactInstance; + } + + if (task.isCancelled()) { + raiseSoftException( + method, tag + ": ReactInstance task cancelled. " + stageLabel + ". " + reasonLabel); + return currentReactInstance; + } + + if (reactInstance == null) { + raiseSoftException( + method, tag + ": ReactInstance task returned null. " + stageLabel + ". " + reasonLabel); + return currentReactInstance; + } + + if (currentReactInstance != null && reactInstance != currentReactInstance) { + raiseSoftException( + method, + tag + + ": Detected two different ReactInstances. Returning old. " + + stageLabel + + ". " + + reasonLabel); + } + + return reactInstance; + }; + } + /** * The ReactInstance is loaded. Tear it down, and re-create it. * @@ -1197,30 +1252,18 @@ private Task newGetOrCreateReloadTask(String reason) { // TODO(T136397487): Remove after Venice is shipped to 100% raiseSoftException(method, reason); + ReactInstanceTaskUnwrapper reactInstanceTaskUnwrapper = + createReactInstanceUnwraper("Reload", method, reason); + if (mReloadTask == null) { mReloadTask = mReactInstanceTaskRef .get() .continueWithTask( (task) -> { - log(method, "Starting on UI thread"); - - if (task.isFaulted()) { - raiseSoftException( - method, - "ReactInstance task faulted. Reload reason: " + reason, - task.getError()); - } - - if (task.isCancelled()) { - raiseSoftException( - method, "ReactInstance task cancelled. Reload reason: " + reason); - } - - final ReactInstance reactInstance = task.getResult(); - if (reactInstance == null) { - raiseSoftException(method, "ReactInstance is null. Reload reason: " + reason); - } + log(method, "Starting React Native reload"); + final ReactInstance reactInstance = + reactInstanceTaskUnwrapper.unwrap(task, "1: Starting reload"); final ReactContext reactContext = mBridgelessReactContextRef.getNullable(); if (reactContext == null) { @@ -1234,13 +1277,16 @@ private Task newGetOrCreateReloadTask(String reason) { reactContext.onHostPause(); } - return task; + return Task.forResult(reactInstance); }, mUIExecutor) .continueWithTask( task -> { - final ReactInstance reactInstance = task.getResult(); + final ReactInstance reactInstance = + reactInstanceTaskUnwrapper.unwrap(task, "2: Surface shutdown"); + if (reactInstance == null) { + raiseSoftException(method, "Skipping surface shutdown: ReactInstance null"); return task; } @@ -1250,6 +1296,8 @@ private Task newGetOrCreateReloadTask(String reason) { mBGExecutor) .continueWithTask( task -> { + reactInstanceTaskUnwrapper.unwrap(task, "3: Destroying ReactContext"); + log(method, "Removing memory pressure listener"); mMemoryPressureRouter.removeMemoryPressureListener(mMemoryPressureListener); @@ -1271,10 +1319,14 @@ private Task newGetOrCreateReloadTask(String reason) { mUIExecutor) .continueWithTask( task -> { - final ReactInstance reactInstance = task.getResult(); + final ReactInstance reactInstance = + reactInstanceTaskUnwrapper.unwrap(task, "4: Destroying ReactInstance"); - log(method, "Destroying ReactInstance"); - if (reactInstance != null) { + if (reactInstance == null) { + raiseSoftException( + method, "Skipping ReactInstance.destroy(): ReactInstance null"); + } else { + log(method, "Destroying ReactInstance"); reactInstance.destroy(); } @@ -1291,23 +1343,30 @@ private Task newGetOrCreateReloadTask(String reason) { return newGetOrCreateReactInstanceTask(); }, mBGExecutor) - .onSuccess( + .continueWithTask( task -> { - final ReactInstance reactInstance = task.getResult(); + final ReactInstance reactInstance = + reactInstanceTaskUnwrapper.unwrap(task, "5: Restarting surfaces"); + if (reactInstance == null) { - return null; + raiseSoftException(method, "Skipping surface restart: ReactInstance null"); + return task; } startAttachedSurfaces(method, reactInstance); - return reactInstance; + + return task; }, mBGExecutor) .continueWithTask( task -> { if (task.isFaulted()) { + Exception fault = task.getError(); raiseSoftException( method, - "Failed to re-created ReactInstance. Task faulted. Reload reason: " + "Error during reload. ReactInstance task faulted. Fault reason: " + + fault.getMessage() + + ". Reload reason: " + reason, task.getError()); } @@ -1315,7 +1374,7 @@ private Task newGetOrCreateReloadTask(String reason) { if (task.isCancelled()) { raiseSoftException( method, - "Failed to re-created ReactInstance. Task cancelled. Reload reason: " + "Error during reload. ReactInstance task cancelled. Reload reason: " + reason); } @@ -1349,31 +1408,19 @@ private Task newGetOrCreateDestroyTask(final String reason, @Nullable Exce // TODO(T136397487): Remove after Venice is shipped to 100% raiseSoftException(method, reason, ex); + ReactInstanceTaskUnwrapper reactInstanceTaskUnwrapper = + createReactInstanceUnwraper("Destroy", method, reason); + if (mDestroyTask == null) { mDestroyTask = mReactInstanceTaskRef .get() .continueWithTask( task -> { - log(method, "Destroying ReactInstance on UI Thread"); + log(method, "Starting React Native destruction"); - if (task.isFaulted()) { - raiseSoftException( - method, - "ReactInstance task faulted. Destroy reason: " + reason, - task.getError()); - } - - if (task.isCancelled()) { - raiseSoftException( - method, "ReactInstance task cancelled. Destroy reason: " + reason); - } - - final ReactInstance reactInstance = task.getResult(); - if (reactInstance == null) { - raiseSoftException( - method, "ReactInstance is null. Destroy reason: " + reason); - } + final ReactInstance reactInstance = + reactInstanceTaskUnwrapper.unwrap(task, "1: Starting destroy"); // Step 1: Destroy DevSupportManager if (mUseDevSupport) { @@ -1392,13 +1439,16 @@ private Task newGetOrCreateDestroyTask(final String reason, @Nullable Exce log(method, "Move ReactHost to onHostDestroy()"); mReactLifecycleStateManager.moveToOnHostDestroy(reactContext); - return task; + return Task.forResult(reactInstance); }, mUIExecutor) .continueWithTask( task -> { - final ReactInstance reactInstance = task.getResult(); + final ReactInstance reactInstance = + reactInstanceTaskUnwrapper.unwrap(task, "2: Stopping surfaces"); + if (reactInstance == null) { + raiseSoftException(method, "Skipping surface shutdown: ReactInstance null"); return task; } @@ -1413,6 +1463,8 @@ private Task newGetOrCreateDestroyTask(final String reason, @Nullable Exce mBGExecutor) .continueWithTask( task -> { + reactInstanceTaskUnwrapper.unwrap(task, "3: Destroying ReactContext"); + final ReactContext reactContext = mBridgelessReactContextRef.getNullable(); if (reactContext == null) { @@ -1437,10 +1489,15 @@ private Task newGetOrCreateDestroyTask(final String reason, @Nullable Exce return task; }, mUIExecutor) - .continueWith( + .continueWithTask( task -> { - final ReactInstance reactInstance = task.getResult(); - if (reactInstance != null) { + final ReactInstance reactInstance = + reactInstanceTaskUnwrapper.unwrap(task, "3: Destroying ReactInstance"); + + if (reactInstance == null) { + raiseSoftException( + method, "Skipping ReactInstance.destroy(): ReactInstance null"); + } else { log(method, "Destroying ReactInstance"); reactInstance.destroy(); } @@ -1456,9 +1513,30 @@ private Task newGetOrCreateDestroyTask(final String reason, @Nullable Exce log(method, "Resetting destroy task ref"); mDestroyTask = null; - return null; + return task; }, - mBGExecutor); + mBGExecutor) + .continueWith( + task -> { + if (task.isFaulted()) { + Exception fault = task.getError(); + raiseSoftException( + method, + "React destruction failed. ReactInstance task faulted. Fault reason: " + + fault.getMessage() + + ". Destroy reason: " + + reason, + task.getError()); + } + + if (task.isCancelled()) { + raiseSoftException( + method, + "React destruction failed. ReactInstance task cancelled. Destroy reason: " + + reason); + } + return null; + }); } return mDestroyTask; diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/internal/bolts/Task.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/internal/bolts/Task.java index 07092aec6597ea..87deb28a08a681 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/internal/bolts/Task.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/internal/bolts/Task.java @@ -7,6 +7,7 @@ package com.facebook.react.bridgeless.internal.bolts; +import androidx.annotation.Nullable; import com.facebook.react.interfaces.TaskInterface; import java.util.ArrayList; import java.util.Collection; @@ -187,7 +188,7 @@ public boolean waitForCompletion(long duration, TimeUnit timeUnit) throws Interr /** Creates a completed task with the given value. */ @SuppressWarnings("unchecked") - public static Task forResult(TResult value) { + public static Task forResult(@Nullable TResult value) { if (value == null) { return (Task) TASK_NULL; }