Skip to content

Commit

Permalink
Remove main queue execution of constantsToExport
Browse files Browse the repository at this point in the history
Summary:
## Context
- If a NativeModule requires main queue setup, its `constantsToExport` method is executed on the main queue.
- In the TurboModule system, `constantsToExport` or `getConstants` is treated like a regular synchronous NativeModule method. Therefore, it's always executed on the JS thread.

This difference in behaviour is dangerous when we're A/B testing the TurboModule infra: One could write a NativeModule that requires main queue setup, and have it expose constants that access objects/state only accessible on the UI thread. This NativeModule would work fine in the legacy infra, which could be the case if the NativeModule author is testing locally. But once it ships to prod, it may run with the TurboModule system, and crash the application. To mitigate this risk, I'm removing this special main queue execution of `constantsToExport` from the legacy infrastructure.

## Consequences
- If a NativeModule's `constantsToExport` method accesses objects/state only accessible on the UI thread, it must do so by explicitly scheduling work on the main thread. I wrote up a codemod to fix this for our OSS modules: D21797048.
- Eagerly initialized NativeModules that required main queue setup had their constants calculated eagerly. After the changes in this diff, those NativeModules will have their constants calculated lazily. I don't think this is a big deal because only a handful of NativeModules are eagerly initialized, and eagerly initialized NativeModules are going away anyway.

Changelog:
[iOS][Removed] - Main queue execution of constantsToExport in NativeModules requiring main queue setup

Reviewed By: fkgozali

Differential Revision: D21829091

fbshipit-source-id: df21fd5fd2ef45a291c07400f360bba801ae290f
  • Loading branch information
RSNara authored and facebook-github-bot committed Jun 3, 2020
1 parent 39d6773 commit d7ac21c
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 2 deletions.
4 changes: 4 additions & 0 deletions React/Base/RCTModuleData.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#import <Foundation/Foundation.h>

#import <React/RCTInvalidating.h>
#import "RCTDefines.h"

@protocol RCTBridgeMethod;
@protocol RCTBridgeModule;
Expand Down Expand Up @@ -94,3 +95,6 @@ typedef id<RCTBridgeModule> (^RCTBridgeModuleProvider)(void);
@property (nonatomic, assign, readonly) BOOL implementsPartialBatchDidFlush;

@end

RCT_EXTERN void RCTSetIsMainQueueExecutionOfConstantsToExportDisabled(BOOL val);
RCT_EXTERN BOOL RCTIsMainQueueExecutionOfConstantsToExportDisabled();
15 changes: 14 additions & 1 deletion React/Base/RCTModuleData.mm
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ int32_t getUniqueId()
return counter++;
}
}
static BOOL isMainQueueExecutionOfConstantToExportDisabled = NO;

void RCTSetIsMainQueueExecutionOfConstantsToExportDisabled(BOOL val)
{
isMainQueueExecutionOfConstantToExportDisabled = val;
}

BOOL RCTIsMainQueueExecutionOfConstantsToExportDisabled()
{
return isMainQueueExecutionOfConstantToExportDisabled;
}

@implementation RCTModuleData {
NSDictionary<NSString *, id> *_constantsToExport;
Expand Down Expand Up @@ -389,7 +400,8 @@ - (void)gatherConstants
* require.
*/
BridgeNativeModulePerfLogger::moduleJSRequireEndingStart([moduleName UTF8String]);
if (_requiresMainQueueSetup) {

if (!RCTIsMainQueueExecutionOfConstantsToExportDisabled() && _requiresMainQueueSetup) {
if (!RCTIsMainQueue()) {
RCTLogWarn(@"Required dispatch_sync to load constants for %@. This may lead to deadlocks", _moduleClass);
}
Expand All @@ -400,6 +412,7 @@ - (void)gatherConstants
} else {
_constantsToExport = [_instance constantsToExport] ?: @{};
}

RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"");
} else {
/**
Expand Down
4 changes: 3 additions & 1 deletion React/CxxBridge/RCTCxxBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -907,7 +907,9 @@ - (void)_prepareModulesWithDispatchGroup:(dispatch_group_t)dispatchGroup
if (self.valid && ![moduleData.moduleClass isSubclassOfClass:[RCTCxxModule class]]) {
[self->_performanceLogger appendStartForTag:RCTPLNativeModuleMainThread];
(void)[moduleData instance];
[moduleData gatherConstants];
if (!RCTIsMainQueueExecutionOfConstantsToExportDisabled()) {
[moduleData gatherConstants];
}
[self->_performanceLogger appendStopForTag:RCTPLNativeModuleMainThread];
}
};
Expand Down

0 comments on commit d7ac21c

Please sign in to comment.