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<String> 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
This commit is contained in:
Ramanpreet Nara
2020-04-28 17:44:15 -07:00
committed by Facebook GitHub Bot
parent 00b3cbfa97
commit e171c2b92a
@@ -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.
*
* <p>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.
*
* <p>Note: After we've started cleanup, getModule will always return null.
* <p>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<TurboModule> getModules() {
final List<TurboModuleHolder> turboModuleHolders = new ArrayList<>();
@@ -305,11 +306,17 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry {
mTurboModuleCleanupStarted = true;
}
final Set<String> turboModuleNames = new HashSet<>(mTurboModuleHolders.keySet());
for (final Map.Entry<String, TurboModuleHolder> 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()