Skip to content

FIRApp: thread safety fixes #2639

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 26, 2019
Merged

FIRApp: thread safety fixes #2639

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94017c0
FIRApp: thread safety fixes
maksymmalyhin Mar 26, 2019
4d2e39b
FIRApp: Revert using GULMutableDictionary, use `@synchronized(self)` …
maksymmalyhin Mar 26, 2019
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
20 changes: 11 additions & 9 deletions Firebase/Core/FIRApp.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#import "Private/FIRLogger.h"
#import "Private/FIROptionsInternal.h"

#import <GoogleUtilities/GULMutableDictionary.h>

NSString *const kFIRServiceAdMob = @"AdMob";
NSString *const kFIRServiceAuth = @"Auth";
NSString *const kFIRServiceAuthUI = @"AuthUI";
Expand Down Expand Up @@ -96,9 +98,9 @@ @implementation FIRApp
// This is necessary since our custom getter prevents `_options` from being created.
@synthesize options = _options;

static NSMutableDictionary *sAllApps;
static GULMutableDictionary *sAllApps;
Copy link
Member

@paulb777 paulb777 Mar 26, 2019

Choose a reason for hiding this comment

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

Is GULMutableDictionary needed? Would it be sufficient to do the accesses with synchronized(self) like some of the sAllApps accesses already are?

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 26, 2019

Choose a reason for hiding this comment

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

Just adding synchronized(self) would be sufficient.
My intention here was to fix the current issue and help to prevent it happening in the future. With just synchronized(self) in case if new logic is added, a developer would have to remember to add synchronized(self). With GULMutableDictionary new logic around sAllApps and sLibraryVersions will be most likely thread safe without additional efforts.

Please, let me know if I am missing something.

Copy link
Member

@paulb777 paulb777 Mar 26, 2019

Choose a reason for hiding this comment

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

It seems like this might be a partial complete fix. I'm a bit worried about destabilizing low-level code that we don't have great tests for. I wonder if today we should do the minimal fix of adding the missing sychronized(self)'s and for the next release do the complete fix of everything in this PR plus making a new GoogleUtilities subspec plus removing the redundant synchronized selfs?

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 26, 2019

Choose a reason for hiding this comment

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

Sounds reasonable.
The only thing I am worried about with sychronized(self) - I don't know the code well enough to be sure that additional sychronized(self) will not introduce a deadlock. Let me take a look at the code one more time.

static FIRApp *sDefaultApp;
static NSMutableDictionary *sLibraryVersions;
static GULMutableDictionary *sLibraryVersions;

+ (void)configure {
FIROptions *options = [FIROptions defaultOptions];
Expand Down Expand Up @@ -214,8 +216,7 @@ + (NSDictionary *)allApps {
if (!sAllApps) {
FIRLogError(kFIRLoggerCore, @"I-COR000005", @"No app has been configured yet.");
}
NSDictionary *dict = [NSDictionary dictionaryWithDictionary:sAllApps];
return dict;
return [sAllApps dictionary];
}
}

Expand Down Expand Up @@ -255,7 +256,7 @@ - (void)deleteApp:(FIRAppVoidBoolCallback)completion {

+ (void)addAppToAppDictionary:(FIRApp *)app {
if (!sAllApps) {
sAllApps = [NSMutableDictionary dictionary];
sAllApps = [[GULMutableDictionary alloc] init];
}
if ([app configureCore]) {
sAllApps[app.name] = app;
Expand Down Expand Up @@ -423,7 +424,7 @@ + (void)sendNotificationsToSDKs:(FIRApp *)app {

// This is the new way of sending information to SDKs.
// TODO: Do we want this on a background thread, maybe?
for (Class<FIRLibrary> library in sRegisteredAsConfigurable) {
for (Class<FIRLibrary> library in [sRegisteredAsConfigurable copy]) {
[library configureWithApp:app];
}
}
Expand Down Expand Up @@ -477,7 +478,7 @@ + (void)registerLibrary:(nonnull NSString *)name withVersion:(nonnull NSString *
if ([name rangeOfCharacterFromSet:disallowedSet].location == NSNotFound &&
[version rangeOfCharacterFromSet:disallowedSet].location == NSNotFound) {
if (!sLibraryVersions) {
sLibraryVersions = [[NSMutableDictionary alloc] init];
sLibraryVersions = [[GULMutableDictionary alloc] init];
}
sLibraryVersions[name] = version;
} else {
Expand Down Expand Up @@ -514,9 +515,10 @@ + (void)registerInternalLibrary:(nonnull Class<FIRLibrary>)library
}

+ (NSString *)firebaseUserAgent {
NSDictionary *libraryVersions = [sLibraryVersions dictionary];
NSMutableArray<NSString *> *libraries =
[[NSMutableArray<NSString *> alloc] initWithCapacity:sLibraryVersions.count];
for (NSString *libraryName in sLibraryVersions) {
[[NSMutableArray<NSString *> alloc] initWithCapacity:libraryVersions.count];
for (NSString *libraryName in libraryVersions) {
[libraries
addObject:[NSString stringWithFormat:@"%@/%@", libraryName, sLibraryVersions[libraryName]]];
}
Expand Down
1 change: 1 addition & 0 deletions FirebaseCore.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Firebase Core includes FIRApp and FIROptions which provide central configuration
s.framework = 'Foundation'
s.dependency 'GoogleUtilities/Environment', '~> 5.2'
s.dependency 'GoogleUtilities/Logger', '~> 5.2'
s.dependency 'GoogleUtilities/Network', '~> 5.2'
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 26, 2019

Choose a reason for hiding this comment

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

Unfortunately this is the only way to make GULMutableDictionary available in FirebaseCore. Probably, we should consider moving GULMutableDictionary to a separate pod (potentially together with similar classes).

s.pod_target_xcconfig = {
'GCC_C_LANGUAGE_STANDARD' => 'c99',
'GCC_PREPROCESSOR_DEFINITIONS' =>
Expand Down