From 9c35b5b8c4710dfe6a4b689a5565aa78ae5b37d3 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Fri, 31 Jul 2020 12:45:48 -0700 Subject: [PATCH] Dispatch promise methods to the NativeModules thread Summary: In D17480605 (https://github.com/facebook/react-native/commit/689233b018bd533a7eecd38e38a7fb84b849cf88), 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 --- .../turbomodule/core/TurboModuleManager.java | 6 ++- .../jni/ReactCommon/TurboModuleManager.cpp | 5 +- .../core/jni/ReactCommon/TurboModuleManager.h | 3 +- ReactCommon/turbomodule/core/BUCK | 4 +- .../core/platform/android/JavaTurboModule.cpp | 49 +++++++++++++++++-- .../core/platform/android/JavaTurboModule.h | 7 +++ 6 files changed, 64 insertions(+), 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 index bf1db2e6476..ff656b93a48 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 @@ -61,7 +61,8 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { jsContext.get(), (CallInvokerHolderImpl) jsCallInvokerHolder, (CallInvokerHolderImpl) nativeCallInvokerHolder, - delegate); + delegate, + false); installJSIBindings(); mEagerInitModuleNames = @@ -278,7 +279,8 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry { long jsContext, CallInvokerHolderImpl jsCallInvokerHolder, CallInvokerHolderImpl nativeCallInvokerHolder, - TurboModuleManagerDelegate tmmDelegate); + TurboModuleManagerDelegate tmmDelegate, + boolean enablePromiseAsyncDispatch); 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 index c2a3b76df36..d074ba5b121 100644 --- 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 @@ -39,10 +39,13 @@ jni::local_ref TurboModuleManager::initHybrid( jlong jsContext, jni::alias_ref jsCallInvokerHolder, jni::alias_ref nativeCallInvokerHolder, - jni::alias_ref delegate) { + jni::alias_ref 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/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 index 4c5a89eb43c..899d2a1d1cf 100644 --- 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 @@ -30,7 +30,8 @@ class TurboModuleManager : public jni::HybridClass { jlong jsContext, jni::alias_ref jsCallInvokerHolder, jni::alias_ref nativeCallInvokerHolder, - jni::alias_ref delegate); + jni::alias_ref delegate, + bool enablePromiseAsyncDispatch); static void registerNatives(); private: diff --git a/ReactCommon/turbomodule/core/BUCK b/ReactCommon/turbomodule/core/BUCK index 512c16ef811..549ddef0a9e 100644 --- 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 index e79158ee7cc..3d32ecfe1be 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp @@ -28,6 +28,11 @@ JavaTurboModule::JavaTurboModule(const InitParams ¶ms) instance_(jni::make_global(params.instance)), nativeInvoker_(params.nativeInvoker) {} +bool JavaTurboModule::isPromiseAsyncDispatchEnabled_ = false; +void JavaTurboModule::enablePromiseAsyncDispatch(bool enable) { + isPromiseAsyncDispatchEnabled_ = enable; +} + namespace { jni::local_ref 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 index 3a903e49558..642cacb194a 100644 --- 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 instance_; std::shared_ptr nativeInvoker_; + /** + * Experiments + */ + static bool isPromiseAsyncDispatchEnabled_; + JNIArgs convertJSIArgsToJNIArgs( JNIEnv *env, jsi::Runtime &rt,