Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions FirebaseRemoteConfig/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# Unreleased
- [fixed] Fixed a bug where Remote Config does not work after a restore
of a previous backup of the device. (#14459)
- [fixed] Fixed a data race condition on the global database status flag
by synchronizing all read and write operations. (#14715)

# 12.3.0
- [fixed] Add missing GoogleUtilities dependency to fix SwiftPM builds when
building dynamically linked libraries. (#15276)
Expand Down
4 changes: 4 additions & 0 deletions FirebaseRemoteConfig/Sources/FIRRemoteConfig.m
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tusharkhandelwal8 there are several dispatch_syncs going on here. If we're on the main queue when dispatching synchronously, we could hold up the main queue and risk deadlocks. Is there a way to make the synchronization of the property asynchronous?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is fine as-is. The work done on the synchronizing queue is only ever:

  • read a global variable
  • write a global variable
  • nothing (empty block to synchronize)

In the future a more ideal architecture would either be to initialize asynchronously and then overwrite the database with the backup-restored data, or just initialize synchronously on the main thread instead of doing async initialization and then using a locking mechanism to wait for it. @tusharkhandelwal8 if you have the time you could try profiling app launch with this patch to see how the dispatch_sync affects launch times.

Regardless, I'm fine with approving this as a temporary fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion on profiling!
I profiled the app launch with the dispatch_sync patch to assess the performance overhead. The synchronization block almost increased 0.3% of the total measured App Launch Time in the test environment. Given the extremely low impact, this synchronous block seems like a good solution for now.

I agree that a fully synchronous or fully asynchronous initialization would be the long-term architectural goal.

Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ - (instancetype)initWithAppName:(NSString *)appName

// Initialize RCConfigContent if not already.
_configContent = configContent;

// We must ensure the DBManager's asynchronous setup (which sets gIsNewDatabase)
// completes before RCNConfigSettings tries to read that state for the resetUserDefaults logic.
[_DBManager waitForDatabaseOperationQueue];
_settings = [[RCNConfigSettings alloc] initWithDatabaseManager:_DBManager
namespace:_FIRNamespace
firebaseAppName:appName
Expand Down
4 changes: 4 additions & 0 deletions FirebaseRemoteConfig/Sources/RCNConfigDBManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,8 @@ typedef void (^RCNDBLoadCompletion)(BOOL success,

/// Returns true if this a new install of the Config database.
- (BOOL)isNewDatabase;

/// Blocks the calling thread until all pending database operations on the internal serial queue are
/// completed. Used to enforce initialization order.
- (void)waitForDatabaseOperationQueue;
@end
24 changes: 22 additions & 2 deletions FirebaseRemoteConfig/Sources/RCNConfigDBManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
/// The storage sub-directory that the Remote Config database resides in.
static NSString *const RCNRemoteConfigStorageSubDirectory = @"Google/RemoteConfig";

/// Introduce a dedicated serial queue for gIsNewDatabase access.
static dispatch_queue_t gIsNewDatabaseQueue;

/// Remote Config database path for deprecated V0 version.
static NSString *RemoteConfigPathForOldDatabaseV0(void) {
NSArray *dirPaths =
Expand Down Expand Up @@ -82,7 +85,9 @@ static BOOL RemoteConfigCreateFilePathIfNotExist(NSString *filePath) {
}
NSFileManager *fileManager = [NSFileManager defaultManager];
if (![fileManager fileExistsAtPath:filePath]) {
gIsNewDatabase = YES;
dispatch_sync(gIsNewDatabaseQueue, ^{
gIsNewDatabase = YES;
});
NSError *error;
[fileManager createDirectoryAtPath:[filePath stringByDeletingLastPathComponent]
withIntermediateDirectories:YES
Expand Down Expand Up @@ -119,6 +124,8 @@ + (instancetype)sharedInstance {
static dispatch_once_t onceToken;
static RCNConfigDBManager *sharedInstance;
dispatch_once(&onceToken, ^{
gIsNewDatabaseQueue = dispatch_queue_create("com.google.FirebaseRemoteConfig.gIsNewDatabase",
DISPATCH_QUEUE_SERIAL);
sharedInstance = [[RCNConfigDBManager alloc] init];
});
return sharedInstance;
Expand Down Expand Up @@ -1219,7 +1226,20 @@ - (BOOL)logErrorWithSQL:(const char *)SQL
}

- (BOOL)isNewDatabase {
return gIsNewDatabase;
__block BOOL isNew;
dispatch_sync(gIsNewDatabaseQueue, ^{
isNew = gIsNewDatabase;
});
return isNew;
}

- (void)waitForDatabaseOperationQueue {
// This dispatch_sync call ensures that all blocks queued before it on _databaseOperationQueue
// (including the createOrOpenDatabase setup block) execute and complete before this method
// returns.
dispatch_sync(_databaseOperationQueue, ^{
// Empty block forces synchronization.
});
}

@end
Loading