From 1e9d4cde4be3e3919bc47797ad384b06476921cf Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Sat, 19 Oct 2019 02:27:35 -0700 Subject: [PATCH] Wait until everything is destroyed before returning from CatalystInstanceImpl.destroy Summary: CatalystInstanceImpl.destroy does a bunch of stuff in each of the relevant threads (Native Module thread, JS thread, UI thread). This change creates a V1 destroy method (unchanged) and a V2 destroy method. The goal is to resolve (and catch!) race conditions in native modules and JSI modules that could occur during teardown; and mitigate race conditions that occur in RN teardown, like deallocation of C++ objects (scheduler, JS VM, and UIManager for Fabric). Changelog: [Internal] Experiment to fix deallocation race conditions Reviewed By: mdvacca Differential Revision: D18001677 fbshipit-source-id: 5955da0a7b726491c7d749642475f0fba74cce5a --- .../react/bridge/CatalystInstanceImpl.java | 110 ++++++++++++++++++ .../react/config/ReactFeatureFlags.java | 6 + 2 files changed, 116 insertions(+) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index e49a2fe3643..46ad79944ca 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -26,6 +26,7 @@ import com.facebook.react.bridge.queue.ReactQueueConfigurationImpl; import com.facebook.react.bridge.queue.ReactQueueConfigurationSpec; import com.facebook.react.common.ReactConstants; import com.facebook.react.common.annotations.VisibleForTesting; +import com.facebook.react.config.ReactFeatureFlags; import com.facebook.react.module.annotations.ReactModule; import com.facebook.react.turbomodule.core.CallInvokerHolderImpl; import com.facebook.react.turbomodule.core.interfaces.TurboModule; @@ -85,6 +86,8 @@ public class CatalystInstanceImpl implements CatalystInstance { private final String mJsPendingCallsTitleForTrace = "pending_js_calls_instance" + sNextInstanceIdForTrace.getAndIncrement(); private volatile boolean mDestroyed = false; + private volatile boolean mNativeModulesThreadDestructionComplete = false; + private volatile boolean mJSThreadDestructionComplete = false; private final TraceListener mTraceListener; private final JavaScriptModuleRegistry mJSModuleRegistry; private final JSBundleLoader mJSBundleLoader; @@ -333,6 +336,18 @@ public class CatalystInstanceImpl implements CatalystInstance { Log.d(ReactConstants.TAG, "CatalystInstanceImpl.destroy() start"); UiThreadUtil.assertOnUiThread(); + if (ReactFeatureFlags.useCatalystTeardownV2) { + destroyV2(); + } else { + destroyV1(); + } + } + + @ThreadConfined(UI) + public void destroyV1() { + Log.d(ReactConstants.TAG, "CatalystInstanceImpl.destroyV1() start"); + UiThreadUtil.assertOnUiThread(); + if (mDestroyed) { return; } @@ -408,6 +423,101 @@ public class CatalystInstanceImpl implements CatalystInstance { Systrace.unregisterListener(mTraceListener); } + /** + * Destroys this catalyst instance, waiting for any other threads in ReactQueueConfiguration + * (besides the UI thread) to finish running. Must be called from the UI thread so that we can + * fully shut down other threads. + */ + @ThreadConfined(UI) + public void destroyV2() { + Log.d(ReactConstants.TAG, "CatalystInstanceImpl.destroyV2() start"); + UiThreadUtil.assertOnUiThread(); + + if (mDestroyed) { + return; + } + + // TODO: tell all APIs to shut down + ReactMarker.logMarker(ReactMarkerConstants.DESTROY_CATALYST_INSTANCE_START); + mDestroyed = true; + mNativeModulesThreadDestructionComplete = false; + mJSThreadDestructionComplete = false; + + mNativeModulesQueueThread.runOnQueue( + new Runnable() { + @Override + public void run() { + Log.d("CatalystInstanceImpl", ".destroy on native modules thread"); + mNativeModuleRegistry.notifyJSInstanceDestroy(); + + // Notifies all JSI modules that they are being destroyed, including the FabricUIManager + // and Fabric Scheduler + mJSIModuleRegistry.notifyJSInstanceDestroy(); + boolean wasIdle = (mPendingJSCalls.getAndSet(0) == 0); + if (!mBridgeIdleListeners.isEmpty()) { + for (NotThreadSafeBridgeIdleDebugListener listener : mBridgeIdleListeners) { + if (!wasIdle) { + listener.onTransitionToBridgeIdle(); + } + listener.onBridgeDestroyed(); + } + } + + mNativeModulesThreadDestructionComplete = true; + Log.d("CatalystInstanceImpl", ".destroy on native modules thread finished"); + } + }); + + getReactQueueConfiguration() + .getJSQueueThread() + .runOnQueue( + new Runnable() { + @Override + public void run() { + Log.d("CatalystInstanceImpl", ".destroy on JS thread"); + // We need to destroy the TurboModuleManager on the JS Thread + if (mTurboModuleManagerJSIModule != null) { + mTurboModuleManagerJSIModule.onCatalystInstanceDestroy(); + } + + mJSThreadDestructionComplete = true; + Log.d("CatalystInstanceImpl", ".destroy on JS thread finished"); + } + }); + + // Wait until destruction is complete + long waitStartTime = System.currentTimeMillis(); + while (!mNativeModulesThreadDestructionComplete || !mJSThreadDestructionComplete) { + // Never wait here, blocking the UI thread, for more than 100ms + if ((System.currentTimeMillis() - waitStartTime) > 100) { + Log.w( + ReactConstants.TAG, + "CatalystInstanceImpl.destroy() timed out waiting for Native Modules and JS thread teardown"); + break; + } + } + + // Kill non-UI threads from neutral third party + // potentially expensive, so don't run on UI thread + + // contextHolder is used as a lock to guard against + // other users of the JS VM having the VM destroyed + // underneath them, so notify them before we reset + // Native + mJavaScriptContextHolder.clear(); + + // Imperatively destruct the C++ CatalystInstance rather than + // wait for the JVM's GC to free it. + mHybridData.resetNative(); + + getReactQueueConfiguration().destroy(); + Log.d(ReactConstants.TAG, "CatalystInstanceImpl.destroy() end"); + ReactMarker.logMarker(ReactMarkerConstants.DESTROY_CATALYST_INSTANCE_END); + + // This is a noop if the listener was not yet registered. + Systrace.unregisterListener(mTraceListener); + } + @Override public boolean isDestroyed() { return mDestroyed; diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index b95d637f04f..934994a62a6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -55,4 +55,10 @@ public class ReactFeatureFlags { * ...$$PropsSetter} class will be used instead. */ public static boolean useViewManagerDelegates = false; + + /** + * Should this application use Catalyst Teardown V2? This is an experiment to use a V2 of the + * CatalystInstanceImpl `destroy` method. + */ + public static boolean useCatalystTeardownV2 = false; }