diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java index 7c88d5e8a..d4476d537 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java @@ -176,12 +176,18 @@ public abstract class BaseJavaModule implements NativeModule { private final int mJSArgumentsNeeded; private final String mTraceName; - public JavaMethod(Method method) { + public JavaMethod(Method method, boolean isSync) { mMethod = method; mMethod.setAccessible(true); + + if (isSync) { + mType = METHOD_TYPE_SYNC; + } + + // TODO: create these lazily Class[] parameterTypes = method.getParameterTypes(); mArgumentExtractors = buildArgumentExtractors(parameterTypes); - mSignature = buildSignature(parameterTypes); + mSignature = buildSignature(mMethod, parameterTypes, isSync); // Since native methods are invoked from a message queue executed on a single thread, it is // save to allocate only one arguments object per method that can be reused across calls mArguments = new Object[parameterTypes.length]; @@ -197,9 +203,16 @@ public abstract class BaseJavaModule implements NativeModule { return mSignature; } - private String buildSignature(Class[] paramTypes) { - StringBuilder builder = new StringBuilder(paramTypes.length); - builder.append("v."); + private String buildSignature(Method method, Class[] paramTypes, boolean isSync) { + StringBuilder builder = new StringBuilder(paramTypes.length + 2); + + if (isSync) { + builder.append(returnTypeToChar(method.getReturnType())); + builder.append('.'); + } else { + builder.append("v."); + } + for (int i = 0; i < paramTypes.length; i++) { Class paramClass = paramTypes[i]; if (paramClass == ExecutorToken.class) { @@ -212,7 +225,9 @@ public abstract class BaseJavaModule implements NativeModule { } else if (paramClass == Promise.class) { Assertions.assertCondition( i == paramTypes.length - 1, "Promise must be used as last parameter only"); - mType = METHOD_TYPE_PROMISE; + if (!isSync) { + mType = METHOD_TYPE_PROMISE; + } } builder.append(paramTypeToChar(paramClass)); } @@ -352,6 +367,7 @@ public abstract class BaseJavaModule implements NativeModule { * Determines how the method is exported in JavaScript: * METHOD_TYPE_ASYNC for regular methods * METHOD_TYPE_PROMISE for methods that return a promise object to the caller. + * METHOD_TYPE_SYNC for sync methods */ @Override public String getType() { @@ -359,82 +375,28 @@ public abstract class BaseJavaModule implements NativeModule { } } - public class SyncJavaHook implements SyncNativeHook { - - private Method mMethod; - private final String mSignature; - - public SyncJavaHook(Method method) { - mMethod = method; - mMethod.setAccessible(true); - mSignature = buildSignature(method); - } - - public Method getMethod() { - return mMethod; - } - - public String getSignature() { - return mSignature; - } - - private String buildSignature(Method method) { - Class[] paramTypes = method.getParameterTypes(); - StringBuilder builder = new StringBuilder(paramTypes.length + 2); - - builder.append(returnTypeToChar(method.getReturnType())); - builder.append('.'); - - for (int i = 0; i < paramTypes.length; i++) { - Class paramClass = paramTypes[i]; - if (paramClass == ExecutorToken.class) { - if (!BaseJavaModule.this.supportsWebWorkers()) { - throw new RuntimeException( - "Module " + BaseJavaModule.this + " doesn't support web workers, but " + - mMethod.getName() + - " takes an ExecutorToken."); - } - } else if (paramClass == Promise.class) { - Assertions.assertCondition( - i == paramTypes.length - 1, "Promise must be used as last parameter only"); - } - builder.append(paramTypeToChar(paramClass)); - } - - return builder.toString(); - } - } - private @Nullable Map mMethods; - private @Nullable Map mHooks; private void findMethods() { if (mMethods == null) { Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "findMethods"); mMethods = new HashMap<>(); - mHooks = new HashMap<>(); Method[] targetMethods = getClass().getDeclaredMethods(); for (Method targetMethod : targetMethods) { - if (targetMethod.getAnnotation(ReactMethod.class) != null) { + ReactMethod annotation = targetMethod.getAnnotation(ReactMethod.class); + if (annotation != null) { String methodName = targetMethod.getName(); - if (mHooks.containsKey(methodName) || mMethods.containsKey(methodName)) { + if (mMethods.containsKey(methodName)) { // We do not support method overloading since js sees a function as an object regardless // of number of params. throw new IllegalArgumentException( - "Java Module " + getName() + " sync method name already registered: " + methodName); + "Java Module " + getName() + " method name already registered: " + methodName); } - mMethods.put(methodName, new JavaMethod(targetMethod)); - } - if (targetMethod.getAnnotation(ReactSyncHook.class) != null) { - String methodName = targetMethod.getName(); - if (mHooks.containsKey(methodName) || mMethods.containsKey(methodName)) { - // We do not support method overloading since js sees a function as an object regardless - // of number of params. - throw new IllegalArgumentException( - "Java Module " + getName() + " sync method name already registered: " + methodName); - } - mHooks.put(methodName, new SyncJavaHook(targetMethod)); + mMethods.put( + methodName, + new JavaMethod(targetMethod, + annotation.isBlockingSynchronousMethod())); } } Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); @@ -454,11 +416,6 @@ public abstract class BaseJavaModule implements NativeModule { return null; } - public final Map getSyncHooks() { - findMethods(); - return assertNotNull(mHooks); - } - @Override public void initialize() { // do nothing diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java index 3bf1264dc..b92ed701a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java @@ -24,13 +24,6 @@ public interface NativeModule { String getType(); } - /** - * A method that can be called from JS synchronously on the JS thread and return a result. - * @see ReactSyncHook - */ - interface SyncNativeHook { - } - /** * @return the name of this module. This will be the name used to {@code require()} this module * from javascript. diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMethod.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMethod.java index 0cc44f6e9..821cc79a5 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMethod.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMethod.java @@ -14,13 +14,28 @@ import java.lang.annotation.Retention; import static java.lang.annotation.RetentionPolicy.RUNTIME; /** - * Annotation which is used to mark methods that are exposed to - * Catalyst. This applies to derived classes of {@link - * BaseJavaModule}, which will generate a list of exported methods by - * searching for those which are annotated with this annotation and - * adding a JS callback for each. + * Annotation which is used to mark methods that are exposed to React Native. + * + * This applies to derived classes of {@link BaseJavaModule}, which will generate a list + * of exported methods by searching for those which are annotated with this annotation + * and adding a JS callback for each. */ @Retention(RUNTIME) public @interface ReactMethod { - + /** + * Whether the method can be called from JS synchronously **on the JS thread**, + * possibly returning a result. + * + * WARNING: in the vast majority of cases, you should leave this to false which allows + * your native module methods to be called asynchronously: calling methods + * synchronously can have strong performance penalties and introduce threading-related + * bugs to your native modules. + * + * In order to support remote debugging, both the method args and return type must be + * serializable to JSON: this means that we only support the same args as + * {@link ReactMethod}, and the hook can only be void or return JSON values (e.g. bool, + * number, String, {@link WritableMap}, or {@link WritableArray}). Calling these + * methods when running under the websocket executor is currently not supported. + */ + boolean isBlockingSynchronousMethod() default false; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSyncHook.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSyncHook.java deleted file mode 100644 index d48b6c9f3..000000000 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSyncHook.java +++ /dev/null @@ -1,31 +0,0 @@ -/** - * Copyright (c) 2015-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - */ - -package com.facebook.react.bridge; - -import java.lang.annotation.Retention; - -import static java.lang.annotation.RetentionPolicy.RUNTIME; - -/** - * Annotation for a method in a {@link NativeModule} that can be called from JS synchronously **on - * the JS thread**, possibly returning a result. - * - * In order to support remote debugging, both the method args and return type must be serializable - * to JSON: this means that we only support the same args as {@link ReactMethod}, and the hook can - * only be void or return JSON values (e.g. bool, number, String, {@link WritableMap}, or - * {@link WritableArray}). - * - * In the vast majority of cases, you should use {@link ReactMethod} which allows your native module - * methods to be called asynchronously: calling methods synchronously can have strong performance - * penalties and introduce threading-related bugs to your native modules. - */ -@Retention(RUNTIME) -public @interface ReactSyncHook { -} diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JavaModuleWrapper.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JavaModuleWrapper.java index 0922eb9d1..45c197c5f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JavaModuleWrapper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JavaModuleWrapper.java @@ -9,6 +9,7 @@ package com.facebook.react.cxxbridge; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -19,6 +20,7 @@ import com.facebook.react.bridge.CatalystInstance; import com.facebook.react.bridge.ExecutorToken; import com.facebook.react.bridge.NativeArray; import com.facebook.react.bridge.NativeModuleLogger; +import com.facebook.react.bridge.NativeModule; import com.facebook.react.bridge.ReadableNativeArray; import com.facebook.react.bridge.WritableNativeArray; import com.facebook.react.bridge.WritableNativeMap; @@ -38,6 +40,10 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; /* package */ class JavaModuleWrapper { @DoNotStrip public class MethodDescriptor { + @DoNotStrip + Method method; + @DoNotStrip + String signature; @DoNotStrip String name; @DoNotStrip @@ -46,7 +52,7 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; private final CatalystInstance mCatalystInstance; private final ModuleHolder mModuleHolder; - private final ArrayList mMethods; + private final ArrayList mMethods; public JavaModuleWrapper(CatalystInstance catalystinstance, ModuleHolder moduleHolder) { mCatalystInstance = catalystinstance; @@ -67,19 +73,21 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; @DoNotStrip public List getMethodDescriptors() { ArrayList descs = new ArrayList<>(); - - for (Map.Entry entry : - getModule().getMethods().entrySet()) { + for (Map.Entry entry : + getModule().getMethods().entrySet()) { MethodDescriptor md = new MethodDescriptor(); md.name = entry.getKey(); md.type = entry.getValue().getType(); BaseJavaModule.JavaMethod method = (BaseJavaModule.JavaMethod) entry.getValue(); + if (md.type == BaseJavaModule.METHOD_TYPE_SYNC) { + md.signature = method.getSignature(); + md.method = method.getMethod(); + } mMethods.add(method); descs.add(md); } - return descs; } diff --git a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h index 106ed3142..dfaf6df7b 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h +++ b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h @@ -2,9 +2,9 @@ #pragma once +#include #include -#include #include "CxxModuleWrapper.h" namespace facebook { diff --git a/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java index dec87ebe3..eeb095b2b 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java @@ -11,7 +11,6 @@ package com.facebook.react.bridge; import java.util.Map; -import com.facebook.react.bridge.Arguments; import com.facebook.react.bridge.ReadableNativeArray; import org.junit.Before; @@ -55,20 +54,28 @@ public class BaseJavaModuleTest { regularMethod.invoke(null, null, mArguments); } - @Test(expected = NativeArgumentsParseException.class) - public void testCallAsyncMethodWithoutEnoughArgs() throws Exception { - BaseJavaModule.NativeMethod asyncMethod = mMethods.get("asyncMethod"); + @Test + public void testCallMethodWithEnoughArgs() { + BaseJavaModule.NativeMethod regularMethod = mMethods.get("regularMethod"); Mockito.stub(mArguments.size()).toReturn(2); - asyncMethod.invoke(null, null, mArguments); + regularMethod.invoke(null, null, mArguments); } - @Test() - public void testCallAsyncMethodWithEnoughArgs() throws Exception { + @Test + public void testCallAsyncMethodWithEnoughArgs() { + // Promise block evaluates to 2 args needing to be passed from JS BaseJavaModule.NativeMethod asyncMethod = mMethods.get("asyncMethod"); Mockito.stub(mArguments.size()).toReturn(3); asyncMethod.invoke(null, null, mArguments); } + @Test + public void testCallSyncMethod() { + BaseJavaModule.NativeMethod syncMethod = mMethods.get("syncMethod"); + Mockito.stub(mArguments.size()).toReturn(2); + syncMethod.invoke(null, null, mArguments); + } + private static class MethodsModule extends BaseJavaModule { @Override public String getName() { @@ -76,11 +83,14 @@ public class BaseJavaModuleTest { } @ReactMethod - public void regularMethod(String a, int b) { - } + public void regularMethod(String a, int b) {} @ReactMethod - public void asyncMethod(int a, Promise p) { + public void asyncMethod(int a, Promise p) {} + + @ReactMethod(isBlockingSynchronousMethod = true) + public int syncMethod(int a, int b) { + return a + b; } } }