Fix RemoteConfig multithreading (#8626)

* Fix RemoteConfig multithreading

* RCNConfigContent multithreading tests

av/rc-fix-threads-stuck

* RCNConfigContent test style

av/rc-fix-threads-stuck

* RCNConfigContent test more asserts

av/rc-fix-threads-stuck

Additional info: it seems safe to put XCTAssert in non-main queue.
Apple example: https://developer.apple.com/documentation/xctest/asynchronous_tests_and_expectations/testing_asynchronous_operations_with_expectations?language=objc
Callback in example "is executed on the delegate queue" https://developer.apple.com/documentation/foundation/nsurlsession/1410330-datataskwithurl?language=objc

Co-authored-by: Artem Volkov <[email protected]>

Author: ArtVolkov

FirebaseRemoteConfig/Sources/RCNConfigContent.m

@@ -41,18 +41,18 @@ @implementation RCNConfigContent {
   RCNConfigDBManager *_DBManager;
   /// Current bundle identifier;
   NSString *_bundleIdentifier;
-  /// Dispatch semaphore to block all config reads until we have read from the database. This only
+  /// Blocks all config reads until we have read from the database. This only
   /// potentially blocks on the first read. Should be a no-wait for all subsequent reads once we
   /// have data read into memory from the database.
-  dispatch_semaphore_t _configLoadFromDBSemaphore;
+  dispatch_group_t _dispatch_group;
   /// Boolean indicating if initial DB load of fetched,active and default config has succeeded.
   BOOL _isConfigLoadFromDBCompleted;
   /// Boolean indicating that the load from database has initiated at least once.
   BOOL _isDatabaseLoadAlreadyInitiated;
 }
 
 /// Default timeout when waiting to read data from database.
-static const NSTimeInterval kDatabaseLoadTimeoutSecs = 30.0;
+const NSTimeInterval kDatabaseLoadTimeoutSecs = 30.0;
 
 /// Singleton instance of RCNConfigContent.
 + (instancetype)sharedInstance {
@@ -86,7 +86,8 @@ - (instancetype)initWithDBManager:(RCNConfigDBManager *)DBManager {
       _bundleIdentifier = @"";
     }
     _DBManager = DBManager;
-    _configLoadFromDBSemaphore = dispatch_semaphore_create(0);
+    // Waits for both config and Personalization data to load.
+    _dispatch_group = dispatch_group_create();
     [self loadConfigFromMainTable];
   }
   return self;
@@ -112,22 +113,25 @@ - (void)loadConfigFromMainTable {
   NSAssert(!_isDatabaseLoadAlreadyInitiated, @"Database load has already been initiated");
   _isDatabaseLoadAlreadyInitiated = true;
 
+  dispatch_group_enter(_dispatch_group);
   [_DBManager
       loadMainWithBundleIdentifier:_bundleIdentifier
                  completionHandler:^(BOOL success, NSDictionary *fetchedConfig,
                                      NSDictionary *activeConfig, NSDictionary *defaultConfig) {
                    self->_fetchedConfig = [fetchedConfig mutableCopy];
                    self->_activeConfig = [activeConfig mutableCopy];
                    self->_defaultConfig = [defaultConfig mutableCopy];
-                   dispatch_semaphore_signal(self->_configLoadFromDBSemaphore);
+                   dispatch_group_leave(self->_dispatch_group);
                  }];
 
   // TODO(karenzeng): Refactor personalization to be returned in loadMainWithBundleIdentifier above
+  dispatch_group_enter(_dispatch_group);
   [_DBManager loadPersonalizationWithCompletionHandler:^(
                   BOOL success, NSDictionary *fetchedPersonalization,
                   NSDictionary *activePersonalization, NSDictionary *defaultConfig) {
     self->_fetchedPersonalization = [fetchedPersonalization copy];
     self->_activePersonalization = [activePersonalization copy];
+    dispatch_group_leave(self->_dispatch_group);
   }];
 }
 
@@ -359,12 +363,12 @@ - (NSDictionary *)getConfigAndMetadataForNamespace:(NSString *)FIRNamespace {
 /// configs until load is done.
 /// @return Database load completion status.
 - (BOOL)checkAndWaitForInitialDatabaseLoad {
-  /// Wait on semaphore until done. This should be a no-op for subsequent calls.
+  /// Wait until load is done. This should be a no-op for subsequent calls.
   if (!_isConfigLoadFromDBCompleted) {
-    long result = dispatch_semaphore_wait(
-        _configLoadFromDBSemaphore,
+    intptr_t isErrorOrTimeout = dispatch_group_wait(
+        _dispatch_group,
         dispatch_time(DISPATCH_TIME_NOW, (int64_t)(kDatabaseLoadTimeoutSecs * NSEC_PER_SEC)));
-    if (result != 0) {
+    if (isErrorOrTimeout) {
       FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000048",
                   @"Timed out waiting for fetched config to be loaded from DB");
       return false;

FirebaseRemoteConfig/Sources/RCNConfigDBManager.m

@@ -868,6 +868,9 @@ - (void)loadPersonalizationWithCompletionHandler:(RCNDBLoadCompletion)handler {
   dispatch_async(_databaseOperationQueue, ^{
     RCNConfigDBManager *strongSelf = weakSelf;
     if (!strongSelf) {
+      dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+        handler(NO, [NSMutableDictionary new], [NSMutableDictionary new], nil);
+      });
       return;
     }
 
@@ -898,7 +901,7 @@ - (void)loadPersonalizationWithCompletionHandler:(RCNDBLoadCompletion)handler {
     }
 
     if (handler) {
-      dispatch_async(dispatch_get_main_queue(), ^{
+      dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
         handler(YES, fetchedPersonalization, activePersonalization, nil);
       });
     }
@@ -972,6 +975,9 @@ - (void)loadMainWithBundleIdentifier:(NSString *)bundleIdentifier
   dispatch_async(_databaseOperationQueue, ^{
     RCNConfigDBManager *strongSelf = weakSelf;
     if (!strongSelf) {
+      dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+        handler(NO, [NSDictionary new], [NSDictionary new], [NSDictionary new]);
+      });
       return;
     }
     __block NSDictionary *fetchedConfig =

FirebaseRemoteConfig/Tests/Unit/RCNConfigContentTest.m

@@ -25,7 +25,36 @@
 #import "FirebaseRemoteConfig/Tests/Unit/RCNTestUtilities.h"
 
 @interface RCNConfigContent (Testing)
-- (void)checkAndWaitForInitialDatabaseLoad;
+- (BOOL)checkAndWaitForInitialDatabaseLoad;
+@end
+
+extern const NSTimeInterval kDatabaseLoadTimeoutSecs;
+@interface RCNConfigDBManagerMock : RCNConfigDBManager
+@property(nonatomic, assign) BOOL isLoadMainCompleted;
+@property(nonatomic, assign) BOOL isLoadPersonalizationCompleted;
+@end
+@implementation RCNConfigDBManagerMock
+- (void)createOrOpenDatabase {
+}
+- (void)loadMainWithBundleIdentifier:(NSString *)bundleIdentifier
+                   completionHandler:(RCNDBLoadCompletion)handler {
+  double justSmallDelay = 0.008;
+  XCTAssertTrue(justSmallDelay < kDatabaseLoadTimeoutSecs);
+  dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(justSmallDelay * NSEC_PER_SEC)),
+                 dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+                   self.isLoadMainCompleted = YES;
+                   handler(YES, nil, nil, nil);
+                 });
+}
+- (void)loadPersonalizationWithCompletionHandler:(RCNDBLoadCompletion)handler {
+  double justOtherSmallDelay = 0.009;
+  XCTAssertTrue(justOtherSmallDelay < kDatabaseLoadTimeoutSecs);
+  dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(justOtherSmallDelay * NSEC_PER_SEC)),
+                 dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+                   self.isLoadPersonalizationCompleted = YES;
+                   handler(YES, nil, nil, nil);
+                 });
+}
 @end
 
 @interface RCNConfigContentTest : XCTestCase {
@@ -262,4 +291,39 @@ - (void)testCopyFromDictionaryUpdatesActiveConfig {
                         [_configContent.activeConfig[@"dummy_namespace"][@"new_key"] dataValue]);
 }
 
+- (void)testCheckAndWaitForInitialDatabaseLoad {
+  RCNConfigDBManagerMock *mockDBManager = [[RCNConfigDBManagerMock alloc] init];
+  RCNConfigContent *configContent = [[RCNConfigContent alloc] initWithDBManager:mockDBManager];
+
+  // Check that no one of first three calls of `-checkAndWaitForInitialDatabaseLoad` do not produce
+  // timeout error <begin>
+  XCTestExpectation *expectation1 =
+      [self expectationWithDescription:
+                @"1st `checkAndWaitForInitialDatabaseLoad` return without timeout"];
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+    XCTAssertTrue([configContent checkAndWaitForInitialDatabaseLoad]);
+    [expectation1 fulfill];
+  });
+  XCTestExpectation *expectation2 =
+      [self expectationWithDescription:
+                @"2nd `checkAndWaitForInitialDatabaseLoad` return without timeout"];
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+    XCTAssertTrue([configContent checkAndWaitForInitialDatabaseLoad]);
+    [expectation2 fulfill];
+  });
+
+  XCTAssertTrue([configContent checkAndWaitForInitialDatabaseLoad]);
+  // Check that both `-load...` methods already completed after 1st wait.
+  // This make us sure that both `-loadMainWithBundleIdentifier` and
+  // `-loadPersonalizationWithCompletionHandler` methods synched with
+  // `-checkAndWaitForInitialDatabaseLoad`.
+  XCTAssertTrue(mockDBManager.isLoadMainCompleted);
+  XCTAssertTrue(mockDBManager.isLoadPersonalizationCompleted);
+
+  // Check that no one of first three calls of `-checkAndWaitForInitialDatabaseLoad` do not produce
+  // timeout error <end>.
+  // This make us sure that there no threads "stucked" on `-checkAndWaitForInitialDatabaseLoad`.
+  [self waitForExpectationsWithTimeout:0.5 * kDatabaseLoadTimeoutSecs handler:nil];
+}
+
 @end