From e171c2b92aa5cfe4634edb64c3cd7c9ab6167ce7 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Tue, 28 Apr 2020 17:44:15 -0700 Subject: [PATCH] Fix cleanup bug Summary: When I initially made TurboModuleManager.getModule thread-safe, I unintentionally broken TurboModule cleanup. This diff fixes that mistake. ## Mistake ``` Override public void onCatalystInstanceDestroy() { synchronized (mTurboModuleCleanupLock) { mTurboModuleCleanupStarted = true; } final Set turboModuleNames = new HashSet<>(mTurboModuleHolders.keySet()); for (final String moduleName : turboModuleNames) { // Retrieve the TurboModule, possibly waiting for it to finish instantiating. final TurboModule turboModule = getModule(moduleName); // ERROR! if (turboModule != null) { // TODO(T48014458): Rename this to invalidate() ((NativeModule) turboModule).onCatalystInstanceDestroy(); } } ``` Before calling `getModule(moduleName)`, I set `mTurboModuleCleanupStarted = true`. This assignment makes all calls to `getModule` return `null`, which means that none of the TurboModules were having their `onCatalystInstanceDestroy()` method invoked. Changelog: [Android][Fixed] - TurboModule cleanup Reviewed By: fkgozali Differential Revision: D21290582 fbshipit-source-id: 3645b9a4f8c6f41498ebd9d51f9b5775edf2dbd7 --- .../turbomodule/core/TurboModuleManager.java | 89 ++++++++++--------- 1 file changed, 48 insertions(+), 41 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java index 1b97c5bd62d..d2afb785c8b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java @@ -104,13 +104,38 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { return mEagerInitModuleNames; } + @DoNotStrip + @Nullable + private CxxModuleWrapper getLegacyCxxModule(String moduleName) { + final TurboModule turboModule = getModule(moduleName); + if (!(turboModule instanceof CxxModuleWrapper)) { + return null; + } + + return (CxxModuleWrapper) turboModule; + } + + @DoNotStrip + @Nullable + private TurboModule getJavaModule(String moduleName) { + final TurboModule turboModule = getModule(moduleName); + if (turboModule instanceof CxxModuleWrapper) { + return null; + } + + return turboModule; + } + /** - * TurboModuleHolders are used as locks to ensure that when n threads race to create a - * TurboModule, only the first thread creates that TurboModule. All other n - 1 threads wait until - * the TurboModule is created and initialized. + * Return the TurboModule instance that corresponds to the provided moduleName. + * + *

This method: - Creates and initializes the module if it doesn't already exist. - Returns + * null after TurboModuleManager has been torn down. */ @Nullable - private TurboModuleHolder getOrMaybeCreateTurboModuleHolder(String moduleName) { + public TurboModule getModule(String moduleName) { + TurboModuleHolder moduleHolder; + synchronized (mTurboModuleCleanupLock) { if (mTurboModuleCleanupStarted) { /* @@ -132,24 +157,22 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { if (!mTurboModuleHolders.containsKey(moduleName)) { mTurboModuleHolders.put(moduleName, new TurboModuleHolder()); } - return mTurboModuleHolders.get(moduleName); + + moduleHolder = mTurboModuleHolders.get(moduleName); } + + return getModule(moduleName, moduleHolder); } /** - * If n threads race to create TurboModule x, then only the first thread should create x. All n - - * 1 other threads should wait until x is created and initialized. + * Given a TurboModuleHolder, and the TurboModule's moduleName, return the TurboModule instance. * - *

Note: After we've started cleanup, getModule will always return null. + *

Use the TurboModuleHolder to ensure that if n threads race to create TurboModule x, then + * only the first thread creates x. All n - 1 other threads wait until the x is created and + * initialized. */ @Nullable - public TurboModule getModule(String moduleName) { - final TurboModuleHolder moduleHolder = getOrMaybeCreateTurboModuleHolder(moduleName); - - if (moduleHolder == null) { - return null; - } - + private TurboModule getModule(String moduleName, @NonNull TurboModuleHolder moduleHolder) { boolean shouldCreateModule = false; synchronized (moduleHolder) { @@ -220,28 +243,6 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { } } - @DoNotStrip - @Nullable - private CxxModuleWrapper getLegacyCxxModule(String moduleName) { - final TurboModule turboModule = getModule(moduleName); - if (!(turboModule instanceof CxxModuleWrapper)) { - return null; - } - - return (CxxModuleWrapper) turboModule; - } - - @DoNotStrip - @Nullable - private TurboModule getJavaModule(String moduleName) { - final TurboModule turboModule = getModule(moduleName); - if (turboModule instanceof CxxModuleWrapper) { - return null; - } - - return turboModule; - } - /** Which TurboModules have been created? */ public Collection getModules() { final List turboModuleHolders = new ArrayList<>(); @@ -305,11 +306,17 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { mTurboModuleCleanupStarted = true; } - final Set turboModuleNames = new HashSet<>(mTurboModuleHolders.keySet()); + for (final Map.Entry moduleHolderEntry : + mTurboModuleHolders.entrySet()) { + final String moduleName = moduleHolderEntry.getKey(); + final TurboModuleHolder moduleHolder = moduleHolderEntry.getValue(); - for (final String moduleName : turboModuleNames) { - // Retrieve the TurboModule, possibly waiting for it to finish instantiating. - final TurboModule turboModule = getModule(moduleName); + /** + * ReactNative could start tearing down before this particular TurboModule has been fully + * initialized. In this case, we should wait for initialization to complete, before destroying + * the TurboModule. + */ + final TurboModule turboModule = getModule(moduleName, moduleHolder); if (turboModule != null) { // TODO(T48014458): Rename this to invalidate()