FIRApp: thread safety fixes #2639
Conversation
FirebaseCore.podspec
Outdated
| @@ -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' | |||
There was a problem hiding this comment.
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).
Firebase/Core/FIRApp.m
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
Is GULMutableDictionary needed? Would it be sufficient to do the accesses with synchronized(self) like some of the sAllApps accesses already are?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@paulb777 I update the PR with |
maksymmalyhin
changed the title
|
Glad this was fixed as it was causing an exception in my unit testing where ether |
|
@toohotz We are happy to help! |
|
Seems like I jumped the gun a bit sadly, re ran my unit test after updating to FB Core 5.20.1 (current latest as of now and can confirm I do have the Screenshot for reference @maksymmalyhin I may naively assume (having not dug into the codebase too much) that there might be more than just the synchronization that might be required? |
|
@toohotz I cannot be 100% sure from the information provided, but it doesn't look like Firebase is involved here. I looks like |
|
Turned out to be code that was written but was a bit hidden since the state was hidden by another ivar. The thread mislead me a bit by showing other FIR threads but stripping out the code on app start with just the FB config quickly proved that it was within my own code. Thanks again, appreciate the work |
Fix #2624 :
@synchronized(self)to fix thread safety.