Dispatch promise methods to the NativeModules thread

Summary: In D17480605 (689233b), I made all methods with void return types dispatch to the NativeModules thread. This diff makes the same change to methods with promise return types. **Note:** The changes are disabled for now. I'll add an MC so that we can test this in production in a later diff. Changelog: [Android][Fixed] - Make promise NativeModule methods dispatch to NativeModules thread Reviewed By: PeteTheHeat Differential Revision: D22489338 fbshipit-source-id: d5b030871f9f7b3f48eb111225516521493cb05e
facebook · Jul 31, 2020 · 9c35b5b · 9c35b5b
1 parent aa1d31e
commit 9c35b5b
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 10 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
@@ -61,7 +61,8 @@ public TurboModuleManager(
             jsContext.get(),
             (CallInvokerHolderImpl) jsCallInvokerHolder,
             (CallInvokerHolderImpl) nativeCallInvokerHolder,
-            delegate);
+            delegate,
+            false);
     installJSIBindings();
 
     mEagerInitModuleNames =
@@ -278,7 +279,8 @@ private native HybridData initHybrid(
       long jsContext,
       CallInvokerHolderImpl jsCallInvokerHolder,
       CallInvokerHolderImpl nativeCallInvokerHolder,
-      TurboModuleManagerDelegate tmmDelegate);
+      TurboModuleManagerDelegate tmmDelegate,
+      boolean enablePromiseAsyncDispatch);
 
   private native void installJSIBindings();
 

diff --git a/.../src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp b/.../src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp
@@ -39,10 +39,13 @@ jni::local_ref<TurboModuleManager::jhybriddata> TurboModuleManager::initHybrid(
     jlong jsContext,
     jni::alias_ref<CallInvokerHolder::javaobject> jsCallInvokerHolder,
     jni::alias_ref<CallInvokerHolder::javaobject> nativeCallInvokerHolder,
-    jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate) {
+    jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate,
+    bool enablePromiseAsyncDispatch) {
   auto jsCallInvoker = jsCallInvokerHolder->cthis()->getCallInvoker();
   auto nativeCallInvoker = nativeCallInvokerHolder->cthis()->getCallInvoker();
 
+  JavaTurboModule::enablePromiseAsyncDispatch(enablePromiseAsyncDispatch);
+
   return makeCxxInstance(
       jThis,
       (jsi::Runtime *)jsContext,

diff --git a/...id/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h b/...id/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h
@@ -30,7 +30,8 @@ class TurboModuleManager : public jni::HybridClass<TurboModuleManager> {
       jlong jsContext,
       jni::alias_ref<CallInvokerHolder::javaobject> jsCallInvokerHolder,
       jni::alias_ref<CallInvokerHolder::javaobject> nativeCallInvokerHolder,
-      jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate);
+      jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate,
+      bool enablePromiseAsyncDispatch);
   static void registerNatives();
 
  private:

diff --git a/ReactCommon/turbomodule/core/BUCK b/ReactCommon/turbomodule/core/BUCK
@@ -1,5 +1,5 @@
 load("@fbsource//tools/build_defs/apple:flag_defs.bzl", "OBJC_ARC_PREPROCESSOR_FLAGS", "get_preprocessor_flags_for_build_mode", "get_static_library_ios_flags")
-load("//tools/build_defs/oss:rn_defs.bzl", "ANDROID", "APPLE", "react_native_target", "react_native_xplat_target", "rn_xplat_cxx_library", "subdir_glob")
+load("//tools/build_defs/oss:rn_defs.bzl", "ANDROID", "APPLE", "FBJNI_TARGET", "react_native_target", "react_native_xplat_target", "rn_xplat_cxx_library", "subdir_glob")
 
 rn_xplat_cxx_library(
     name = "core",
@@ -22,6 +22,7 @@ rn_xplat_cxx_library(
     ],
     fbandroid_deps = [
         react_native_target("jni/react/jni:jni"),
+        FBJNI_TARGET,
     ],
     fbandroid_exported_headers = subdir_glob(
         [
@@ -40,7 +41,6 @@ rn_xplat_cxx_library(
     ],
     fbobjc_inherited_buck_flags = get_static_library_ios_flags(),
     fbobjc_preprocessor_flags = OBJC_ARC_PREPROCESSOR_FLAGS + get_preprocessor_flags_for_build_mode(),
-    force_static = True,
     ios_deps = [
         "//xplat/FBBaseLite:FBBaseLite",
         "//xplat/js/react-native-github:RCTCxxModule",

diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp
@@ -28,6 +28,11 @@ JavaTurboModule::JavaTurboModule(const InitParams &params)
       instance_(jni::make_global(params.instance)),
       nativeInvoker_(params.nativeInvoker) {}
 
+bool JavaTurboModule::isPromiseAsyncDispatchEnabled_ = false;
+void JavaTurboModule::enablePromiseAsyncDispatch(bool enable) {
+  isPromiseAsyncDispatchEnabled_ = enable;
+}
+
 namespace {
 jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
     jsi::Function &&function,
@@ -227,7 +232,8 @@ JNIArgs JavaTurboModule::convertJSIArgsToJNIArgs(
 
   auto makeGlobalIfNecessary =
       [&globalRefs, env, valueKind](jobject obj) -> jobject {
-    if (valueKind == VoidKind) {
+    if (valueKind == VoidKind ||
+        (valueKind == PromiseKind && isPromiseAsyncDispatchEnabled_)) {
       jobject globalObj = env->NewGlobalRef(obj);
       globalRefs.push_back(globalObj);
       env->DeleteLocalRef(obj);
@@ -586,7 +592,13 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
           runtime,
           jsi::PropNameID::forAscii(runtime, "fn"),
           2,
-          [this, &jargs, argCount, instance, methodID, env](
+          [this,
+           &jargs,
+           &globalRefs,
+           argCount,
+           instance_ = instance_,
+           methodID,
+           env](
               jsi::Runtime &runtime,
               const jsi::Value &thisVal,
               const jsi::Value *promiseConstructorArgs,
@@ -619,8 +631,37 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
             jobject promise = env->NewObject(
                 jPromiseImpl, jPromiseImplConstructor, resolve, reject);
 
-            jargs[argCount].l = promise;
-            env->CallVoidMethodA(instance, methodID, jargs.data());
+            if (isPromiseAsyncDispatchEnabled_) {
+              jobject globalPromise = env->NewGlobalRef(promise);
+
+              globalRefs.push_back(globalPromise);
+              env->DeleteLocalRef(promise);
+
+              jargs[argCount].l = globalPromise;
+
+              nativeInvoker_->invokeAsync(
+                  [jargs, globalRefs, methodID, instance_ = instance_]() 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();
+
+                    env->CallVoidMethodA(
+                        instance_.get(), methodID, jargs.data());
+                    FACEBOOK_JNI_THROW_PENDING_EXCEPTION();
+
+                    for (auto globalRef : globalRefs) {
+                      env->DeleteGlobalRef(globalRef);
+                    }
+                  });
+
+            } else {
+              jargs[argCount].l = promise;
+              env->CallVoidMethodA(instance_.get(), methodID, jargs.data());
+            }
 
             return jsi::Value::undefined();
           });

diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h
@@ -49,10 +49,17 @@ class JSI_EXPORT JavaTurboModule : public TurboModule {
       const jsi::Value *args,
       size_t argCount);
 
+  static void enablePromiseAsyncDispatch(bool enable);
+
  private:
   jni::global_ref<JTurboModule> instance_;
   std::shared_ptr<CallInvoker> nativeInvoker_;
 
+  /**
+   * Experiments
+   */
+  static bool isPromiseAsyncDispatchEnabled_;
+
   JNIArgs convertJSIArgsToJNIArgs(
       JNIEnv *env,
       jsi::Runtime &rt,