Roll out TurboModule Promise Async Dispatch

RSNara · facebook-github-bot · commit 5c4f145e33d9 · 2020-12-17T17:27:09.000-08:00
Summary: ## Context The legacy NativeModule infra implements promise methods using [async NativeModule method calls](https://fburl.com/diffusion/tpkff6vg). In the TurboModule infra, promise methods [treat as sync methods](https://fburl.com/diffusion/yde7xw71), and executed directly on the JavaScript thread. This experiment makes TurboModule promise methods async, and dispatches them to the NativeModules thread, when they're executed from JavaScript. Reviewed By: fkgozali Differential Revision: D25623192 fbshipit-source-id: 2b50d771c5272af3b6edf150054bb3e80cab0040
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
@@ -61,7 +61,6 @@ public TurboModuleManager(
             (CallInvokerHolderImpl) jsCallInvokerHolder,
             (CallInvokerHolderImpl) nativeCallInvokerHolder,
             delegate,
-            ReactFeatureFlags.enableTurboModulePromiseAsyncDispatch,
             ReactFeatureFlags.useTurboModuleJSCodegen);
     installJSIBindings();
 
@@ -295,7 +294,6 @@ private native HybridData initHybrid(
       CallInvokerHolderImpl jsCallInvokerHolder,
       CallInvokerHolderImpl nativeCallInvokerHolder,
       TurboModuleManagerDelegate tmmDelegate,
-      boolean enablePromiseAsyncDispatch,
       boolean enableTurboModuleJSCodegen);
 
   private native void installJSIBindings();
diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp
@@ -43,13 +43,10 @@ jni::local_ref<TurboModuleManager::jhybriddata> TurboModuleManager::initHybrid(
     jni::alias_ref<CallInvokerHolder::javaobject> jsCallInvokerHolder,
     jni::alias_ref<CallInvokerHolder::javaobject> nativeCallInvokerHolder,
     jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate,
-    bool enablePromiseAsyncDispatch,
     bool enableJSCodegen) {
   auto jsCallInvoker = jsCallInvokerHolder->cthis()->getCallInvoker();
   auto nativeCallInvoker = nativeCallInvokerHolder->cthis()->getCallInvoker();
 
-  JavaTurboModule::enablePromiseAsyncDispatch(enablePromiseAsyncDispatch);
-
   return makeCxxInstance(
       jThis,
       runtimeExecutor->cthis()->get(),
diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h
@@ -33,7 +33,6 @@ class TurboModuleManager : public jni::HybridClass<TurboModuleManager> {
       jni::alias_ref<CallInvokerHolder::javaobject> jsCallInvokerHolder,
       jni::alias_ref<CallInvokerHolder::javaobject> nativeCallInvokerHolder,
       jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate,
-      bool enablePromiseAsyncDispatch,
       bool enableJSCodegen);
   static void registerNatives();
 
diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp
@@ -51,11 +51,6 @@ JavaTurboModule::~JavaTurboModule() {
   });
 }
 
-bool JavaTurboModule::isPromiseAsyncDispatchEnabled_ = false;
-void JavaTurboModule::enablePromiseAsyncDispatch(bool enable) {
-  isPromiseAsyncDispatchEnabled_ = enable;
-}
-
 JavaTurboModule::JavaTurboModule(
     const InitParams &params,
     TurboModuleSchema &&schema)
@@ -322,8 +317,7 @@ JNIArgs JavaTurboModule::convertJSIArgsToJNIArgs(
 
   auto makeGlobalIfNecessary =
       [&globalRefs, env, valueKind](jobject obj) -> jobject {
-    if (valueKind == VoidKind ||
-        (valueKind == PromiseKind && isPromiseAsyncDispatchEnabled_)) {
+    if (valueKind == VoidKind || valueKind == PromiseKind) {
       jobject globalObj = env->NewGlobalRef(obj);
       globalRefs.push_back(globalObj);
       env->DeleteLocalRef(obj);
@@ -491,9 +485,7 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
   const char *methodName = methodNameStr.c_str();
   const char *moduleName = name_.c_str();
 
-  bool isMethodSync =
-      !(valueKind == VoidKind ||
-        (valueKind == PromiseKind && isPromiseAsyncDispatchEnabled_));
+  bool isMethodSync = !(valueKind == VoidKind || valueKind == PromiseKind);
 
   if (isMethodSync) {
     TMPL::syncMethodCallStart(moduleName, methodName);
@@ -834,60 +826,48 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
             const char *moduleName = moduleNameStr.c_str();
             const char *methodName = methodNameStr.c_str();
 
-            if (isPromiseAsyncDispatchEnabled_) {
-              jobject globalPromise = env->NewGlobalRef(promise);
-
-              globalRefs.push_back(globalPromise);
-              env->DeleteLocalRef(promise);
-
-              jargs[argCount].l = globalPromise;
-              TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName);
-              TMPL::asyncMethodCallDispatch(moduleName, methodName);
-
-              nativeInvoker_->invokeAsync(
-                  [jargs,
-                   globalRefs,
-                   methodID,
-                   instance_ = instance_,
-                   moduleNameStr,
-                   methodNameStr,
-                   id = getUniqueId()]() mutable -> void {
-                    /**
-                     * TODO(ramanpreet): Why do we have to require the
-                     * environment again? Why does JNI crash when we use the env
-                     * from the upper scope?
-                     */
-                    JNIEnv *env = jni::Environment::current();
-                    const char *moduleName = moduleNameStr.c_str();
-                    const char *methodName = methodNameStr.c_str();
-
-                    TMPL::asyncMethodCallExecutionStart(
-                        moduleName, methodName, id);
-                    env->CallVoidMethodA(
-                        instance_.get(), methodID, jargs.data());
-                    try {
-                      FACEBOOK_JNI_THROW_PENDING_EXCEPTION();
-                    } catch (...) {
-                      TMPL::asyncMethodCallExecutionFail(
-                          moduleName, methodName, id);
-                      throw;
-                    }
-
-                    for (auto globalRef : globalRefs) {
-                      env->DeleteGlobalRef(globalRef);
-                    }
-                    TMPL::asyncMethodCallExecutionEnd(
+            jobject globalPromise = env->NewGlobalRef(promise);
+
+            globalRefs.push_back(globalPromise);
+            env->DeleteLocalRef(promise);
+
+            jargs[argCount].l = globalPromise;
+            TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName);
+            TMPL::asyncMethodCallDispatch(moduleName, methodName);
+
+            nativeInvoker_->invokeAsync(
+                [jargs,
+                 globalRefs,
+                 methodID,
+                 instance_ = instance_,
+                 moduleNameStr,
+                 methodNameStr,
+                 id = getUniqueId()]() mutable -> void {
+                  /**
+                   * TODO(ramanpreet): Why do we have to require the
+                   * environment again? Why does JNI crash when we use the env
+                   * from the upper scope?
+                   */
+                  JNIEnv *env = jni::Environment::current();
+                  const char *moduleName = moduleNameStr.c_str();
+                  const char *methodName = methodNameStr.c_str();
+
+                  TMPL::asyncMethodCallExecutionStart(
+                      moduleName, methodName, id);
+                  env->CallVoidMethodA(instance_.get(), methodID, jargs.data());
+                  try {
+                    FACEBOOK_JNI_THROW_PENDING_EXCEPTION();
+                  } catch (...) {
+                    TMPL::asyncMethodCallExecutionFail(
                         moduleName, methodName, id);
-                  });
-
-            } else {
-              jargs[argCount].l = promise;
-              TMPL::syncMethodCallArgConversionEnd(moduleName, methodName);
-              TMPL::syncMethodCallExecutionStart(moduleName, methodName);
-              env->CallVoidMethodA(instance_.get(), methodID, jargs.data());
-              TMPL::syncMethodCallExecutionEnd(moduleName, methodName);
-              TMPL::syncMethodCallReturnConversionStart(moduleName, methodName);
-            }
+                    throw;
+                  }
+
+                  for (auto globalRef : globalRefs) {
+                    env->DeleteGlobalRef(globalRef);
+                  }
+                  TMPL::asyncMethodCallExecutionEnd(moduleName, methodName, id);
+                });
 
             return jsi::Value::undefined();
           });
@@ -896,12 +876,8 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
           Promise.callAsConstructor(runtime, promiseConstructorArg);
       checkJNIErrorForMethodCall();
 
-      if (isPromiseAsyncDispatchEnabled_) {
-        TMPL::asyncMethodCallEnd(moduleName, methodName);
-      } else {
-        TMPL::syncMethodCallReturnConversionEnd(moduleName, methodName);
-        TMPL::syncMethodCallEnd(moduleName, methodName);
-      }
+      TMPL::asyncMethodCallEnd(moduleName, methodName);
+
       return promise;
     }
     default:
diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h
@@ -55,7 +55,6 @@ class JSI_EXPORT JavaTurboModule : public TurboModule {
       const jsi::Value *args,
       size_t argCount);
 
-  static void enablePromiseAsyncDispatch(bool enable);
   jsi::Value get(jsi::Runtime &runtime, const jsi::PropNameID &propName)
       override;
 
@@ -64,11 +63,6 @@ class JSI_EXPORT JavaTurboModule : public TurboModule {
   std::shared_ptr<CallInvoker> nativeInvoker_;
   folly::Optional<TurboModuleSchema> turboModuleSchema_;
 
-  /**
-   * Experiments
-   */
-  static bool isPromiseAsyncDispatchEnabled_;
-
   JNIArgs convertJSIArgsToJNIArgs(
       JNIEnv *env,
       jsi::Runtime &rt,
