From d7b37c405013926c2bb4fc98f1575bebf690aaff Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Fri, 17 Mar 2017 06:55:37 -0700 Subject: [PATCH] Remove JsToNativeBridge's nativeQueue Reviewed By: mhorowitz Differential Revision: D4589737 fbshipit-source-id: 3b2730417d99c4f98cfaad386bc50328f2551592 --- React/CxxBridge/RCTCxxBridge.mm | 81 +++++-------------- .../jni/xreact/jni/CatalystInstanceImpl.cpp | 42 ++++++---- .../jni/xreact/jni/CatalystInstanceImpl.h | 2 + .../main/jni/xreact/jni/JavaModuleWrapper.cpp | 42 ++++++---- .../main/jni/xreact/jni/JavaModuleWrapper.h | 13 ++- .../jni/xreact/jni/ModuleRegistryBuilder.cpp | 10 ++- .../jni/xreact/jni/ModuleRegistryBuilder.h | 5 +- ReactCommon/cxxreact/CxxNativeModule.cpp | 52 +++++------- ReactCommon/cxxreact/CxxNativeModule.h | 11 ++- ReactCommon/cxxreact/Instance.cpp | 14 +--- ReactCommon/cxxreact/Instance.h | 1 - ReactCommon/cxxreact/NativeToJsBridge.cpp | 48 +++++------ ReactCommon/cxxreact/NativeToJsBridge.h | 1 - 13 files changed, 142 insertions(+), 180 deletions(-) diff --git a/React/CxxBridge/RCTCxxBridge.mm b/React/CxxBridge/RCTCxxBridge.mm index 16b8782ec0..3e776b6123 100644 --- a/React/CxxBridge/RCTCxxBridge.mm +++ b/React/CxxBridge/RCTCxxBridge.mm @@ -101,70 +101,25 @@ namespace { // is not the JS thread. C++ modules don't use RCTNativeModule, so this little // adapter does the work. -class QueueNativeModule : public NativeModule { +class DispatchMessageQueueThread : public MessageQueueThread { public: - QueueNativeModule(RCTCxxBridge *bridge, - std::unique_ptr module) - : bridge_(bridge) - , module_(std::move(module)) - // Create new queue (store queueName, as it isn't retained by dispatch_queue) - , queueName_("com.facebook.react." + module_->getName() + "Queue") - , queue_(dispatch_queue_create(queueName_.c_str(), DISPATCH_QUEUE_SERIAL)) {} + DispatchMessageQueueThread(RCTModuleData *moduleData) + : moduleData_(moduleData) {} - std::string getName() override { - return module_->getName(); - } - - std::vector getMethods() override { - return module_->getMethods(); - } - - folly::dynamic getConstants() override { - return module_->getConstants(); - } - - bool supportsWebWorkers() override { - return module_->supportsWebWorkers(); - } - - void invoke(ExecutorToken token, unsigned int reactMethodId, - folly::dynamic &¶ms) override { - __weak RCTCxxBridge *bridge = bridge_; - auto module = module_; - auto cparams = std::make_shared(std::move(params)); - dispatch_async(queue_, ^{ - if (!bridge || !bridge.valid) { - return; - } - - module->invoke(token, reactMethodId, std::move(*cparams)); + void runOnQueue(std::function&& func) override { + dispatch_async(moduleData_.methodQueue, [func=std::move(func)] { + func(); }); } - - MethodCallResult callSerializableNativeHook( - ExecutorToken token, unsigned int reactMethodId, - folly::dynamic &&args) override { - return module_->callSerializableNativeHook(token, reactMethodId, std::move(args)); + void runOnQueueSync(std::function&& func) override { + LOG(FATAL) << "Unsupported operation"; + } + void quitSynchronous() override { + LOG(FATAL) << "Unsupported operation"; } private: - RCTCxxBridge *bridge_; - std::shared_ptr module_; - std::string queueName_; - dispatch_queue_t queue_; -}; - - -// This is a temporary hack. The cxx bridge assumes a single native -// queue handles all native method calls, but that's false on ios. So -// this is a no-thread passthrough, and queues are handled in -// RCTNativeModule. A similar refactoring should be done on the Java -// bridge. -class InlineMessageQueueThread : public MessageQueueThread { -public: - void runOnQueue(std::function&& func) override { func(); } - void runOnQueueSync(std::function&& func) override { func(); } - void quitSynchronous() override {} + RCTModuleData *moduleData_; }; } @@ -551,12 +506,13 @@ struct RCTInstanceCallback : public InstanceCallback { if (![moduleData hasInstance]) { continue; } - modules.emplace_back( - new QueueNativeModule(self, std::make_unique( - _reactInstance, [moduleData.name UTF8String], - [moduleData] { return [(RCTCxxModule *)(moduleData.instance) move]; }))); + modules.emplace_back(std::make_unique( + _reactInstance, + [moduleData.name UTF8String], + [moduleData] { return [(RCTCxxModule *)(moduleData.instance) move]; }, + std::make_shared(moduleData))); } else { - modules.emplace_back(new RCTNativeModule(self, moduleData)); + modules.emplace_back(std::make_unique(self, moduleData)); } } @@ -586,7 +542,6 @@ struct RCTInstanceCallback : public InstanceCallback { std::unique_ptr(new RCTInstanceCallback(self)), executorFactory, _jsMessageThread, - std::unique_ptr(new InlineMessageQueueThread), std::move([self _createModuleRegistry])); #if RCT_PROFILE diff --git a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp index a38ed7713f..23b1e4d2d2 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp @@ -41,13 +41,17 @@ class Exception : public jni::JavaClass { class JInstanceCallback : public InstanceCallback { public: - explicit JInstanceCallback(alias_ref jobj) - : jobj_(make_global(jobj)) {} + explicit JInstanceCallback( + alias_ref jobj, + std::shared_ptr messageQueueThread) + : jobj_(make_global(jobj)), messageQueueThread_(std::move(messageQueueThread)) {} void onBatchComplete() override { - static auto method = - ReactCallback::javaClassStatic()->getMethod("onBatchComplete"); - method(jobj_); + messageQueueThread_->runOnQueue([this] { + static auto method = + ReactCallback::javaClassStatic()->getMethod("onBatchComplete"); + method(jobj_); + }); } void incrementPendingJSCalls() override { @@ -61,6 +65,7 @@ class JInstanceCallback : public InstanceCallback { } void decrementPendingJSCalls() override { + jni::ThreadScope guard; static auto method = ReactCallback::javaClassStatic()->getMethod("decrementPendingJSCalls"); method(jobj_); @@ -81,12 +86,11 @@ class JInstanceCallback : public InstanceCallback { return jobj->cthis()->getExecutorToken(jobj); } - void onExecutorStopped(ExecutorToken) override { - // TODO(cjhopman): implement this. - } + void onExecutorStopped(ExecutorToken) override {} private: global_ref jobj_; + std::shared_ptr messageQueueThread_; }; } @@ -97,7 +101,12 @@ jni::local_ref CatalystInstanceImpl::initHybr } CatalystInstanceImpl::CatalystInstanceImpl() - : instance_(folly::make_unique()) {} + : instance_(folly::make_unique()) {} + +CatalystInstanceImpl::~CatalystInstanceImpl() { + // TODO: 16669252: this prevents onCatalystInstanceDestroy from being called + moduleMessageQueue_->quitSynchronous(); +} void CatalystInstanceImpl::registerNatives() { registerHybrid({ @@ -127,11 +136,12 @@ void CatalystInstanceImpl::initializeBridge( // This executor is actually a factory holder. JavaScriptExecutorHolder* jseh, jni::alias_ref jsQueue, - jni::alias_ref moduleQueue, + jni::alias_ref nativeModulesQueue, jni::alias_ref::javaobject> javaModules, jni::alias_ref::javaobject> cxxModules) { // TODO mhorowitz: how to assert here? // Assertions.assertCondition(mBridge == null, "initializeBridge should be called once"); + moduleMessageQueue_ = std::make_shared(nativeModulesQueue); // This used to be: // @@ -149,12 +159,12 @@ void CatalystInstanceImpl::initializeBridge( // don't need jsModuleDescriptions any more, all the way up and down the // stack. - instance_->initializeBridge(folly::make_unique(callback), - jseh->getExecutorFactory(), - folly::make_unique(jsQueue), - folly::make_unique(moduleQueue), - buildModuleRegistry(std::weak_ptr(instance_), - javaModules, cxxModules)); + instance_->initializeBridge( + folly::make_unique(callback, moduleMessageQueue_), + jseh->getExecutorFactory(), + folly::make_unique(jsQueue), + buildModuleRegistry( + std::weak_ptr(instance_), javaModules, cxxModules, moduleMessageQueue_)); } void CatalystInstanceImpl::jniSetSourceURL(const std::string& sourceURL) { diff --git a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.h b/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.h index 804fc927cb..0a811c1e65 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.h +++ b/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.h @@ -28,6 +28,7 @@ class CatalystInstanceImpl : public jni::HybridClass { static constexpr auto kJavaDescriptor = "Lcom/facebook/react/cxxbridge/CatalystInstanceImpl;"; static jni::local_ref initHybrid(jni::alias_ref); + ~CatalystInstanceImpl() override; static void registerNatives(); @@ -74,6 +75,7 @@ class CatalystInstanceImpl : public jni::HybridClass { // This should be the only long-lived strong reference, but every C++ class // will have a weak reference. std::shared_ptr instance_; + std::shared_ptr moduleMessageQueue_; }; }} diff --git a/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.cpp b/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.cpp index e3444346bc..c545f1233a 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.cpp @@ -87,30 +87,36 @@ bool JavaNativeModule::supportsWebWorkers() { } void JavaNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) { - static auto invokeMethod = - wrapper_->getClass()->getMethod("invoke"); - invokeMethod(wrapper_, JExecutorToken::extractJavaPartFromToken(token).get(), static_cast(reactMethodId), - ReadableNativeArray::newObjectCxxArgs(std::move(params)).get()); + messageQueueThread_->runOnQueue([this, token, reactMethodId, params=std::move(params)] { + static auto invokeMethod = wrapper_->getClass()-> + getMethod("invoke"); + invokeMethod(wrapper_, + JExecutorToken::extractJavaPartFromToken(token).get(), + static_cast(reactMethodId), + ReadableNativeArray::newObjectCxxArgs(std::move(params)).get()); + }); } MethodCallResult JavaNativeModule::callSerializableNativeHook(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) { - // TODO: evaluate whether calling through invoke is potentially faster - if (reactMethodId >= syncMethods_.size()) { - throw std::invalid_argument( - folly::to("methodId ", reactMethodId, " out of range [0..", syncMethods_.size(), "]")); - } + // TODO: evaluate whether calling through invoke is potentially faster + if (reactMethodId >= syncMethods_.size()) { + throw std::invalid_argument( + folly::to("methodId ", reactMethodId, " out of range [0..", syncMethods_.size(), "]")); + } - auto& method = syncMethods_[reactMethodId]; - CHECK(method.hasValue() && method->isSyncHook()) << "Trying to invoke a asynchronous method as synchronous hook"; - return method->invoke(instance_, wrapper_->getModule(), token, params); + auto& method = syncMethods_[reactMethodId]; + CHECK(method.hasValue() && method->isSyncHook()) << "Trying to invoke a asynchronous method as synchronous hook"; + return method->invoke(instance_, wrapper_->getModule(), token, params); } NewJavaNativeModule::NewJavaNativeModule( std::weak_ptr instance, - jni::alias_ref wrapper) - : instance_(std::move(instance)), - wrapper_(make_global(wrapper)), - module_(make_global(wrapper->getModule())) { + jni::alias_ref wrapper, + std::shared_ptr messageQueueThread) +: instance_(std::move(instance)) +, wrapper_(make_global(wrapper)) +, module_(make_global(wrapper->getModule())) +, messageQueueThread_(std::move(messageQueueThread)) { auto descs = wrapper_->getMethodDescriptors(); std::string moduleName = getName(); methods_.reserve(descs->size()); @@ -161,7 +167,9 @@ void NewJavaNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId folly::to("methodId ", reactMethodId, " out of range [0..", methods_.size(), "]")); } CHECK(!methods_[reactMethodId].isSyncHook()) << "Trying to invoke a synchronous hook asynchronously"; - invokeInner(token, reactMethodId, std::move(params)); + messageQueueThread_->runOnQueue([this, token, reactMethodId, params=std::move(params)] () mutable { + invokeInner(token, reactMethodId, std::move(params)); + }); } MethodCallResult NewJavaNativeModule::callSerializableNativeHook(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) { diff --git a/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.h b/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.h index 544b6eb167..222765de63 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.h +++ b/ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.h @@ -12,6 +12,7 @@ namespace facebook { namespace react { class Instance; +class MessageQueueThread; struct JMethodDescriptor : public jni::JavaClass { static constexpr auto kJavaDescriptor = @@ -44,8 +45,11 @@ class JavaNativeModule : public NativeModule { public: JavaNativeModule( std::weak_ptr instance, - jni::alias_ref wrapper) - : instance_(std::move(instance)), wrapper_(make_global(wrapper)) {} + jni::alias_ref wrapper, + std::shared_ptr messageQueueThread) + : instance_(std::move(instance)) + , wrapper_(make_global(wrapper)) + , messageQueueThread_(std::move(messageQueueThread)) {} std::string getName() override; folly::dynamic getConstants() override; @@ -57,6 +61,7 @@ class JavaNativeModule : public NativeModule { private: std::weak_ptr instance_; jni::global_ref wrapper_; + std::shared_ptr messageQueueThread_; std::vector> syncMethods_; }; @@ -65,7 +70,8 @@ class NewJavaNativeModule : public NativeModule { public: NewJavaNativeModule( std::weak_ptr instance, - jni::alias_ref wrapper); + jni::alias_ref wrapper, + std::shared_ptr messageQueueThread); std::string getName() override; std::vector getMethods() override; @@ -78,6 +84,7 @@ class NewJavaNativeModule : public NativeModule { std::weak_ptr instance_; jni::global_ref wrapper_; jni::global_ref module_; + std::shared_ptr messageQueueThread_; std::vector methods_; std::vector methodDescriptors_; diff --git a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp index 108091a8df..0dd0c48362 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp @@ -32,14 +32,16 @@ xplat::module::CxxModule::Provider ModuleHolder::getProvider() const { std::unique_ptr buildModuleRegistry( std::weak_ptr winstance, jni::alias_ref::javaobject> javaModules, - jni::alias_ref::javaobject> cxxModules) { + jni::alias_ref::javaobject> cxxModules, + std::shared_ptr moduleMessageQueue) { std::vector> modules; for (const auto& jm : *javaModules) { - modules.emplace_back(folly::make_unique(winstance, jm)); + modules.emplace_back(folly::make_unique( + winstance, jm, moduleMessageQueue)); } for (const auto& cm : *cxxModules) { - modules.emplace_back( - folly::make_unique(winstance, cm->getName(), cm->getProvider())); + modules.emplace_back(folly::make_unique( + winstance, cm->getName(), cm->getProvider(), moduleMessageQueue)); } if (modules.empty()) { return nullptr; diff --git a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.h b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.h index 5544736011..0aeb7d5398 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.h +++ b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.h @@ -12,6 +12,8 @@ namespace facebook { namespace react { +class MessageQueueThread; + class ModuleHolder : public jni::JavaClass { public: static auto constexpr kJavaDescriptor = @@ -24,7 +26,8 @@ class ModuleHolder : public jni::JavaClass { std::unique_ptr buildModuleRegistry( std::weak_ptr winstance, jni::alias_ref::javaobject> javaModules, - jni::alias_ref::javaobject> cxxModules); + jni::alias_ref::javaobject> cxxModules, + std::shared_ptr moduleMessageQueue); } } diff --git a/ReactCommon/cxxreact/CxxNativeModule.cpp b/ReactCommon/cxxreact/CxxNativeModule.cpp index c0cacd0b85..b487588bb7 100644 --- a/ReactCommon/cxxreact/CxxNativeModule.cpp +++ b/ReactCommon/cxxreact/CxxNativeModule.cpp @@ -46,13 +46,6 @@ CxxModule::Callback convertCallback( } -CxxNativeModule::CxxNativeModule(std::weak_ptr instance, - std::string name, - CxxModule::Provider provider) - : instance_(instance) - , name_(std::move(name)) - , provider_(provider) {} - std::string CxxNativeModule::getName() { return name_; } @@ -85,8 +78,8 @@ bool CxxNativeModule::supportsWebWorkers() { void CxxNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) { if (reactMethodId >= methods_.size()) { - throw std::invalid_argument( - folly::to("methodId ", reactMethodId, " out of range [0..", methods_.size(), "]")); + throw std::invalid_argument(folly::to("methodId ", reactMethodId, + " out of range [0..", methods_.size(), "]")); } if (!params.isArray()) { throw std::invalid_argument( @@ -99,25 +92,20 @@ void CxxNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId, fo const auto& method = methods_[reactMethodId]; if (!method.func) { - throw std::runtime_error( - folly::to("Method ", method.name, - " is synchronous but invoked asynchronously")); + throw std::runtime_error(folly::to("Method ", method.name, + " is synchronous but invoked asynchronously")); } if (params.size() < method.callbacks) { - throw std::invalid_argument( - folly::to("Expected ", method.callbacks, " callbacks, but only ", - params.size(), " parameters provided")); + throw std::invalid_argument(folly::to("Expected ", method.callbacks, + " callbacks, but only ", params.size(), " parameters provided")); } if (method.callbacks == 1) { - first = convertCallback( - makeCallback(instance_, token, params[params.size() - 1])); + first = convertCallback(makeCallback(instance_, token, params[params.size() - 1])); } else if (method.callbacks == 2) { - first = convertCallback( - makeCallback(instance_, token, params[params.size() - 2])); - second = convertCallback( - makeCallback(instance_, token, params[params.size() - 1])); + first = convertCallback(makeCallback(instance_, token, params[params.size() - 2])); + second = convertCallback(makeCallback(instance_, token, params[params.size() - 1])); } params.resize(params.size() - method.callbacks); @@ -141,16 +129,18 @@ void CxxNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId, fo // stack. I'm told that will be possible in the future. TODO // mhorowitz #7128529: convert C++ exceptions to Java - try { - method.func(std::move(params), first, second); - } catch (const facebook::xplat::JsArgumentException& ex) { - // This ends up passed to the onNativeException callback. - throw; - } catch (...) { - // This means some C++ code is buggy. As above, we fail hard so the C++ - // developer can debug and fix it. - std::terminate(); - } + messageQueueThread_->runOnQueue([method, params=std::move(params), first, second] () { + try { + method.func(std::move(params), first, second); + } catch (const facebook::xplat::JsArgumentException& ex) { + // This ends up passed to the onNativeException callback. + throw; + } catch (...) { + // This means some C++ code is buggy. As above, we fail hard so the C++ + // developer can debug and fix it. + std::terminate(); + } + }); } MethodCallResult CxxNativeModule::callSerializableNativeHook( diff --git a/ReactCommon/cxxreact/CxxNativeModule.h b/ReactCommon/cxxreact/CxxNativeModule.h index ab6f3df87f..376ad7a198 100644 --- a/ReactCommon/cxxreact/CxxNativeModule.h +++ b/ReactCommon/cxxreact/CxxNativeModule.h @@ -15,8 +15,14 @@ std::function makeCallback( class CxxNativeModule : public NativeModule { public: - CxxNativeModule(std::weak_ptr instance, std::string name, - xplat::module::CxxModule::Provider provider); + CxxNativeModule(std::weak_ptr instance, + std::string name, + xplat::module::CxxModule::Provider provider, + std::shared_ptr messageQueueThread) + : instance_(instance) + , name_(std::move(name)) + , provider_(provider) + , messageQueueThread_(messageQueueThread) {} std::string getName() override; std::vector getMethods() override; @@ -32,6 +38,7 @@ private: std::weak_ptr instance_; std::string name_; xplat::module::CxxModule::Provider provider_; + std::shared_ptr messageQueueThread_; std::unique_ptr module_; std::vector methods_; }; diff --git a/ReactCommon/cxxreact/Instance.cpp b/ReactCommon/cxxreact/Instance.cpp index f7ad78ed4e..300c809dbf 100644 --- a/ReactCommon/cxxreact/Instance.cpp +++ b/ReactCommon/cxxreact/Instance.cpp @@ -33,24 +33,14 @@ void Instance::initializeBridge( std::unique_ptr callback, std::shared_ptr jsef, std::shared_ptr jsQueue, - std::unique_ptr nativeQueue, std::shared_ptr moduleRegistry) { callback_ = std::move(callback); - if (!nativeQueue) { - // TODO pass down a thread/queue from java, instead of creating our own. - - auto queue = folly::make_unique(); - std::thread t(queue->getUnregisteredRunLoop()); - t.detach(); - nativeQueue = std::move(queue); - } jsQueue->runOnQueueSync( - [this, &jsef, moduleRegistry, jsQueue, - nativeQueue=folly::makeMoveWrapper(std::move(nativeQueue))] () mutable { + [this, &jsef, moduleRegistry, jsQueue] () mutable { nativeToJsBridge_ = folly::make_unique( - jsef.get(), moduleRegistry, jsQueue, nativeQueue.move(), callback_); + jsef.get(), moduleRegistry, jsQueue, callback_); std::lock_guard lock(m_syncMutex); m_syncReady = true; diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h index 4b8da4f260..cf1cd2459a 100644 --- a/ReactCommon/cxxreact/Instance.h +++ b/ReactCommon/cxxreact/Instance.h @@ -31,7 +31,6 @@ class Instance { std::unique_ptr callback, std::shared_ptr jsef, std::shared_ptr jsQueue, - std::unique_ptr nativeQueue, std::shared_ptr moduleRegistry); void setSourceURL(std::string sourceURL); diff --git a/ReactCommon/cxxreact/NativeToJsBridge.cpp b/ReactCommon/cxxreact/NativeToJsBridge.cpp index 8c6babfbf4..e863791bc7 100644 --- a/ReactCommon/cxxreact/NativeToJsBridge.cpp +++ b/ReactCommon/cxxreact/NativeToJsBridge.cpp @@ -24,11 +24,9 @@ class JsToNativeBridge : public react::ExecutorDelegate { public: JsToNativeBridge(NativeToJsBridge* nativeToJs, std::shared_ptr registry, - std::unique_ptr nativeQueue, std::shared_ptr callback) : m_nativeToJs(nativeToJs) , m_registry(registry) - , m_nativeQueue(std::move(nativeQueue)) , m_callback(callback) {} void registerExecutor(std::unique_ptr executor, @@ -51,24 +49,25 @@ public: CHECK(m_registry || calls.empty()) << "native module calls cannot be completed with no native modules"; ExecutorToken token = m_nativeToJs->getTokenForExecutor(executor); - m_nativeQueue->runOnQueue([this, token, calls=std::move(calls), isEndOfBatch] () mutable { - m_batchHadNativeModuleCalls = m_batchHadNativeModuleCalls || !calls.empty(); + m_batchHadNativeModuleCalls = m_batchHadNativeModuleCalls || !calls.empty(); - // An exception anywhere in here stops processing of the batch. This - // was the behavior of the Android bridge, and since exception handling - // terminates the whole bridge, there's not much point in continuing. - for (auto& call : react::parseMethodCalls(std::move(calls))) { - m_registry->callNativeMethod( - token, call.moduleId, call.methodId, std::move(call.arguments), call.callId); + // An exception anywhere in here stops processing of the batch. This + // was the behavior of the Android bridge, and since exception handling + // terminates the whole bridge, there's not much point in continuing. + for (auto& call : parseMethodCalls(std::move(calls))) { + m_registry->callNativeMethod( + token, call.moduleId, call.methodId, std::move(call.arguments), call.callId); + } + if (isEndOfBatch) { + // onBatchComplete will be called on the native (module) queue, but + // decrementPendingJSCalls will be called sync. Be aware that the bridge may still + // be processing native calls when the birdge idle signaler fires. + if (m_batchHadNativeModuleCalls) { + m_callback->onBatchComplete(); + m_batchHadNativeModuleCalls = false; } - if (isEndOfBatch) { - if (m_batchHadNativeModuleCalls) { - m_callback->onBatchComplete(); - m_batchHadNativeModuleCalls = false; - } - m_callback->decrementPendingJSCalls(); - } - }); + m_callback->decrementPendingJSCalls(); + } } MethodCallResult callSerializableNativeHook( @@ -78,10 +77,6 @@ public: return m_registry->callSerializableNativeHook(token, moduleId, methodId, std::move(args)); } - void quitQueueSynchronous() { - m_nativeQueue->quitSynchronous(); - } - private: // These methods are always invoked from an Executor. The NativeToJsBridge @@ -91,7 +86,6 @@ private: // valid object during a call to a delegate method from an exectuto. NativeToJsBridge* m_nativeToJs; std::shared_ptr m_registry; - std::unique_ptr m_nativeQueue; std::shared_ptr m_callback; bool m_batchHadNativeModuleCalls = false; }; @@ -100,12 +94,10 @@ NativeToJsBridge::NativeToJsBridge( JSExecutorFactory* jsExecutorFactory, std::shared_ptr registry, std::shared_ptr jsQueue, - std::unique_ptr nativeQueue, std::shared_ptr callback) : m_destroyed(std::make_shared(false)) , m_mainExecutorToken(callback->createExecutorToken()) - , m_delegate(std::make_shared( - this, registry, std::move(nativeQueue), callback)) { + , m_delegate(std::make_shared(this, registry, callback)) { std::unique_ptr mainExecutor = jsExecutorFactory->createJSExecutor(m_delegate, jsQueue); // cached to avoid locked map lookup in the common case @@ -129,7 +121,6 @@ void NativeToJsBridge::loadApplication( startupScript=folly::makeMoveWrapper(std::move(startupScript)), startupScriptSourceURL=std::move(startupScriptSourceURL)] (JSExecutor* executor) mutable { - auto unbundle = unbundleWrap.move(); if (unbundle) { executor->setJSModulesUnbundle(std::move(unbundle)); @@ -322,7 +313,6 @@ ExecutorToken NativeToJsBridge::getTokenForExecutor(JSExecutor& executor) { } void NativeToJsBridge::destroy() { - m_delegate->quitQueueSynchronous(); auto* executorMessageQueueThread = getMessageQueueThread(m_mainExecutorToken); // All calls made through runOnExecutorQueue have an early exit if // m_destroyed is true. Setting this before the runOnQueueSync will cause @@ -331,7 +321,7 @@ void NativeToJsBridge::destroy() { executorMessageQueueThread->runOnQueueSync([this, executorMessageQueueThread] { m_mainExecutor->destroy(); executorMessageQueueThread->quitSynchronous(); - unregisterExecutor(*m_mainExecutor); + m_delegate->unregisterExecutor(*m_mainExecutor); m_mainExecutor = nullptr; }); } diff --git a/ReactCommon/cxxreact/NativeToJsBridge.h b/ReactCommon/cxxreact/NativeToJsBridge.h index 70ef620b88..1c2505f834 100644 --- a/ReactCommon/cxxreact/NativeToJsBridge.h +++ b/ReactCommon/cxxreact/NativeToJsBridge.h @@ -61,7 +61,6 @@ public: JSExecutorFactory* jsExecutorFactory, std::shared_ptr registry, std::shared_ptr jsQueue, - std::unique_ptr nativeQueue, std::shared_ptr callback); virtual ~NativeToJsBridge();