Skip to content

Commit

Permalink
Enhance/fix error reporting in reload and destroy
Browse files Browse the repository at this point in the history 
Summary:
The new reload/create/destroy methods work by chaining tasks together.

This task chain has the type Task<ReactInstance>.

**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<ReactInstance> isn't faulted/cancelled. If the task is faulted/cancelled, a soft exception gets reported, and the current ReactInstance gets returned.

Changelog: [Internal

Differential Revision: D48080779

fbshipit-source-id: 7502709a80360499f6bee2ff35cdcbe89838854d
  • Loading branch information
@RSNara @facebook-github-bot
RSNara authored and facebook-github-bot committed Aug 14, 2023
1 parent 203854f commit 1c9c7fa
Showing 1 changed file with 143 additions and 65 deletions.
208 changes: 143 additions & 65 deletions .../react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,60 @@ private void log(String method) {
@ThreadConfined("ReactHost")
private @Nullable Task<ReactInstance> mReloadTask = null;

private interface ReactInstanceTaskUnwrapper {
@Nullable
ReactInstance unwrap(Task<ReactInstance> 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.
*
Expand All @@ -1151,30 +1205,17 @@ private Task<ReactInstance> 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");
reactInstanceTaskUnwrapper.unwrap(task, "1: Starting reload");

final ReactContext reactContext = mBridgelessReactContextRef.getNullable();
if (reactContext == null) {
Expand All @@ -1193,14 +1234,18 @@ private Task<ReactInstance> newGetOrCreateReloadTask(String reason) {
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;
}

log(method, "Stopping all React Native surfaces");
synchronized (mAttachedSurfaces) {
for (ReactSurface surface : mAttachedSurfaces) {
if (reactInstance != null) {
reactInstance.stopSurface(surface);
}
reactInstance.stopSurface(surface);

surface.clear();
}
Expand All @@ -1211,6 +1256,8 @@ private Task<ReactInstance> newGetOrCreateReloadTask(String reason) {
mBGExecutor)
.continueWithTask(
task -> {
reactInstanceTaskUnwrapper.unwrap(task, "3: Destroying ReactContext");

log(method, "Removing memory pressure listener");
mMemoryPressureRouter.removeMemoryPressureListener(mMemoryPressureListener);

Expand All @@ -1232,10 +1279,14 @@ private Task<ReactInstance> 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();
}

Expand All @@ -1252,35 +1303,44 @@ private Task<ReactInstance> newGetOrCreateReloadTask(String reason) {
return newGetOrCreateReactInstanceTask();
},
mBGExecutor)
.onSuccess(
.continueWithTask(
task -> {
final ReactInstance reactInstance = task.getResult();
if (reactInstance != null) {
log(method, "Restarting previously running React Native Surfaces");

synchronized (mAttachedSurfaces) {
for (ReactSurface surface : mAttachedSurfaces) {
reactInstance.startSurface(surface);
}
final ReactInstance reactInstance =
reactInstanceTaskUnwrapper.unwrap(task, "5: Restarting surfaces");

if (reactInstance == null) {
raiseSoftException(method, "Skipping surface restart: ReactInstance null");
return task;
}

log(method, "Restarting previously running React Native Surfaces");

synchronized (mAttachedSurfaces) {
for (ReactSurface surface : mAttachedSurfaces) {
reactInstance.startSurface(surface);
}
}
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());
}

if (task.isCancelled()) {
raiseSoftException(
method,
"Failed to re-created ReactInstance. Task cancelled. Reload reason: "
"Error during reload. ReactInstance task cancelled. Reload reason: "
+ reason);
}

Expand Down Expand Up @@ -1314,31 +1374,18 @@ private Task<Void> 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");

if (task.isFaulted()) {
raiseSoftException(
method,
"ReactInstance task faulted. Destroy reason: " + reason,
task.getError());
}

if (task.isCancelled()) {
raiseSoftException(
method, "ReactInstance task cancelled. Destroy reason: " + reason);
}
log(method, "Starting React Native destruction");

final ReactInstance reactInstance = task.getResult();
if (reactInstance == null) {
raiseSoftException(
method, "ReactInstance is null. Destroy reason: " + reason);
}
reactInstanceTaskUnwrapper.unwrap(task, "1: Starting destroy");

// Step 1: Destroy DevSupportManager
if (mUseDevSupport) {
Expand All @@ -1362,16 +1409,19 @@ private Task<Void> newGetOrCreateDestroyTask(final String reason, @Nullable Exce
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;
}

// Step 3: Stop all React Native surfaces
log(method, "Stopping all React Native surfaces");
synchronized (mAttachedSurfaces) {
for (ReactSurface surface : mAttachedSurfaces) {
if (reactInstance != null) {
reactInstance.stopSurface(surface);
}

reactInstance.stopSurface(surface);
surface.clear();
}
}
Expand All @@ -1381,6 +1431,8 @@ private Task<Void> newGetOrCreateDestroyTask(final String reason, @Nullable Exce
mBGExecutor)
.continueWithTask(
task -> {
reactInstanceTaskUnwrapper.unwrap(task, "3: Destroying ReactContext");

final ReactContext reactContext = mBridgelessReactContextRef.getNullable();

if (reactContext == null) {
Expand All @@ -1405,10 +1457,15 @@ private Task<Void> 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();
}
Expand All @@ -1424,9 +1481,30 @@ private Task<Void> 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;
Expand Down

0 comments on commit 1c9c7fa

Please sign in to comment.