From 553729f3d684fcdc27af17cdcfaa803f19ca6afd Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Fri, 3 Apr 2020 12:26:10 -0700 Subject: [PATCH] Create method queues for NativeModules that neither provide nor request one Summary: ## Problem: Let `A` be the set of all ObjC NativeModules that neither provide nor reqeust a method queue. The TurboModule system dispatches all method calls to NativeModules in `A` synchronously to the JS thread. Here is the relevant logic: **RCTTurboModule.mm:** Link: https://fburl.com/diffusion/nz9gqje8 ``` jsi::Value performMethodInvocation( // ... ) { // ... dispatch_queue_t methodQueue = NULL; if ([instance_ conformsToProtocol:protocol(RCTBridgeModule)] && [instance_ respondsToSelector:selector(methodQueue)]) { methodQueue = [instance_ performSelector:selector(methodQueue)]; } if (methodQueue == NULL || methodQueue == RCTJSThread) { // This is the default mode of execution: on JS thread. block(); } else if (methodQueue == dispatch_get_main_queue()) { ``` **Why does this end up happening?** 1. NativeModules that request a method queue have `synthesize methodQueue = _methodQueue` in their `implementation` section. This generates a `methodQueue` getter for the NativeModule, and also creates an ivar to back that getter. The TurboModule system generates a `dispatch_queue_t` and uses ObjC's KVC API to write to the ivar. So in the above logic, for NativeModules that provide a method queue, methodQueue will neither be `NULL` nor `RCTJSThread`, so we don't dispatch synchronously to the JS thread. 2. NativeModules that provide a method queue will return something that is not `NULL` or something that is `RCTJSThread`. If they return `NULL`, the infra will throw an error early. If they return `RCTJSThread`, we'll dispatch synchronously to the JS thread, as we should (...wait. For async NativeModule methods that dispatch to `RCTJSThread`, should we dispatch asynchronously to the JS thread, via jsInvoker? **Edit:** Nope: https://fburl.com/diffusion/ivt9b40s.). In all other cases, we dispatch to appropriately to the respective method queue. 3. For NativeModules that neither provide nor request a method queue (i.e: NativeModules in `A`), they don't implement the `methodQueue` selector. Therefore, we dispatch synchronously to the JS thread. ## The fix (Part 1): The first step towards fixing this problem is to generate `dispatch_queue_t`s for NativeModules in `A`. That's what this diff accomplishes. Changelog: [iOS][Fixed] - Create method queue for NativeModules that don't provide nor request one. Reviewed By: fkgozali Differential Revision: D20821054 fbshipit-source-id: 17a73550ad96766c5c7e719e28e1cc879e36465c --- .../platform/ios/RCTTurboModuleManager.mm | 82 ++++++++++++++----- 1 file changed, 63 insertions(+), 19 deletions(-) diff --git a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm index a84df5a3751..791075f2378 100644 --- a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm +++ b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm @@ -11,6 +11,8 @@ #import #import +#import + #import #import #import @@ -23,6 +25,11 @@ using namespace facebook; +/** + * A global variable whose address we use to associate method queues to id objects. + */ +static char kAssociatedMethodQueueKey; + // Fallback lookup since RCT class prefix is sometimes stripped in the existing NativeModule system. // This will be removed in the future. static Class getFallbackClassFromName(const char *name) @@ -266,11 +273,17 @@ static Class getFallbackClassFromName(const char *name) _rctTurboModuleCache.insert({moduleName, module}); } + [self setUpRCTTurboModule:module moduleName:moduleName]; + return module; +} + +- (void)setUpRCTTurboModule:(id)module moduleName:(const char *)moduleName +{ __weak id weakModule = (id)module; __weak RCTBridge *weakBridge = _bridge; id performanceLogger = _performanceLogger; - auto setupTurboModule = ^{ + auto setUpTurboModule = ^{ if (!weakModule) { return; } @@ -325,29 +338,57 @@ static Class getFallbackClassFromName(const char *name) * These modules typically have the following: * `@synthesize methodQueue = _methodQueue` */ - if ([strongModule respondsToSelector:@selector(methodQueue)]) { - [performanceLogger attachMethodQueueToRCTTurboModuleStart:moduleName]; - dispatch_queue_t methodQueue = [strongModule performSelector:@selector(methodQueue)]; - if (!methodQueue) { - NSString *moduleClassName = NSStringFromClass(strongModule.class); - NSString *queueName = [NSString stringWithFormat:@"com.facebook.react.%@Queue", moduleClassName]; - methodQueue = dispatch_queue_create(queueName.UTF8String, DISPATCH_QUEUE_SERIAL); + [performanceLogger attachMethodQueueToRCTTurboModuleStart:moduleName]; + + dispatch_queue_t methodQueue = nil; + BOOL moduleHasMethodQueueGetter = [strongModule respondsToSelector:@selector(methodQueue)]; + + if (moduleHasMethodQueueGetter) { + methodQueue = [strongModule methodQueue]; + } + + /** + * Note: RCTJSThread, which is a valid method queue, is defined as (id)kCFNull. It should rightfully not enter the + * following if condition's block. + */ + if (!methodQueue) { + NSString *methodQueueName = [NSString stringWithFormat:@"com.facebook.react.%sQueue", moduleName]; + methodQueue = dispatch_queue_create(methodQueueName.UTF8String, DISPATCH_QUEUE_SERIAL); + + if (moduleHasMethodQueueGetter) { + /** + * If the module has a method queue getter, two cases are possible: + * - We @synthesized the method queue. In this case, the getter will initially return nil. + * - We had a custom methodQueue function on the NativeModule. If we got this far, then that getter returned + * nil. + * + * Therefore, we do a try/catch and use ObjC's KVC API and try to assign the method queue to the NativeModule. + * In case 1, we'll succeed. In case 2, an exception will be thrown, which we'll ignore. + */ + @try { [(id)strongModule setValue:methodQueue forKey:@"methodQueue"]; } @catch (NSException *exception) { RCTLogError( - @"TM: %@ is returning nil for its methodQueue, which is not " - "permitted. You must either return a pre-initialized " - "queue, or @synthesize the methodQueue to let the bridge " - "create a queue for you.", - moduleClassName); + @"%@ has no setter or ivar for its methodQueue, which is not " + "permitted. You must either @synthesize the bridge property, " + "or provide your own setter method.", + RCTBridgeModuleNameForClass([strongModule class])); } } - - [performanceLogger attachMethodQueueToRCTTurboModuleEnd:moduleName]; } + /** + * Attach method queue to id object. + * This is necessary because the id object can be eagerly created/initialized before the method + * queue is required. The method queue is required for an id for JS -> Native calls. So, we need it + * before we create the id's TurboModule jsi::HostObject in provideTurboModule:. + */ + objc_setAssociatedObject(strongModule, &kAssociatedMethodQueueKey, methodQueue, OBJC_ASSOCIATION_RETAIN); + + [performanceLogger attachMethodQueueToRCTTurboModuleEnd:moduleName]; + /** * NativeModules that implement the RCTFrameUpdateObserver protocol * require registration with RCTDisplayLink. @@ -378,6 +419,11 @@ static Class getFallbackClassFromName(const char *name) [performanceLogger setupRCTTurboModuleEnd:moduleName]; }; + /** + * TODO(T64991809): Fix TurboModule race: + * - When NativeModules that don't require main queue setup are required from different threads, they'll + * concurrently run setUpRCTTurboModule: + */ if ([[module class] respondsToSelector:@selector(requiresMainQueueSetup)] && [[module class] requiresMainQueueSetup]) { /** @@ -388,12 +434,10 @@ static Class getFallbackClassFromName(const char *name) * TODO(T63807674): Investigate the right migration plan off of this */ [_performanceLogger setupRCTTurboModuleDispatch:moduleName]; - RCTUnsafeExecuteOnMainQueueSync(setupTurboModule); + RCTUnsafeExecuteOnMainQueueSync(setUpTurboModule); } else { - setupTurboModule(); + setUpTurboModule(); } - - return module; } - (void)installJSBindingWithRuntime:(jsi::Runtime *)runtime