From f493a316fda963b1cb33aaaf7fdf6182b98d4ff8 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 30 Jun 2020 16:09:03 -0700 Subject: [PATCH] Remove flags around Catalyst teardown Summary: These flags haven't been used in months. They were useful to uncover some race conditions, but will not be iterated further. The Venice project will obviate the concerns that sparked these experiments in the first place. These flags have been hardcoded to false for a while. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D22319204 fbshipit-source-id: 09415f3bb1ca56e15f357210e966d0483ff384f2 --- .../react/bridge/CatalystInstanceImpl.java | 105 ------------------ .../facebook/react/bridge/ReactContext.java | 3 - .../react/config/ReactFeatureFlags.java | 17 +-- 3 files changed, 1 insertion(+), 124 deletions(-) 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 e225306e68b..dae969346aa 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -340,16 +340,6 @@ public class CatalystInstanceImpl implements CatalystInstance { FLog.d(ReactConstants.TAG, "CatalystInstanceImpl.destroy() start"); UiThreadUtil.assertOnUiThread(); - if (ReactFeatureFlags.useCatalystTeardownV2) { - destroyV2(); - } else { - destroyV1(); - } - } - - @ThreadConfined(UI) - public void destroyV1() { - FLog.d(ReactConstants.TAG, "CatalystInstanceImpl.destroyV1() start"); UiThreadUtil.assertOnUiThread(); if (mDestroyed) { @@ -427,101 +417,6 @@ 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() { - FLog.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() { - FLog.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; - FLog.d("CatalystInstanceImpl", ".destroy on native modules thread finished"); - } - }); - - getReactQueueConfiguration() - .getJSQueueThread() - .runOnQueue( - new Runnable() { - @Override - public void run() { - FLog.d("CatalystInstanceImpl", ".destroy on JS thread"); - // We need to destroy the TurboModuleManager on the JS Thread - if (mTurboModuleManagerJSIModule != null) { - mTurboModuleManagerJSIModule.onCatalystInstanceDestroy(); - } - - mJSThreadDestructionComplete = true; - FLog.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) { - FLog.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(); - FLog.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/bridge/ReactContext.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java index 126237e74e8..27d44870176 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java @@ -296,9 +296,6 @@ public class ReactContext extends ContextWrapper { mDestroyed = true; if (mCatalystInstance != null) { mCatalystInstance.destroy(); - if (ReactFeatureFlags.nullifyCatalystInstanceOnDestroy) { - mCatalystInstance = null; - } } } 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 e0882c0cbc8..94e63491530 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -41,22 +41,7 @@ public class ReactFeatureFlags { * inside view manager will be called instead. */ public static boolean useViewManagerDelegatesForCommands = 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; - - /** - * When the ReactContext is destroyed, should the CatalystInstance immediately be nullified? This - * is the safest thing to do since the CatalystInstance shouldn't be used, and should be - * garbage-collected after it's destroyed, but this is a breaking change in that many native - * modules assume that a ReactContext will always have a CatalystInstance. This will be deleted - * and the CatalystInstance will always be destroyed in some future release. - */ - public static boolean nullifyCatalystInstanceOnDestroy = false; - + /** * This react flag enables a custom algorithm for the getChildVisibleRect() method in the classes * ReactViewGroup, ReactHorizontalScrollView and ReactScrollView.