Skip to content
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.

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

FIRApp: thread safety fixes #2639

Merged
merged 2 commits into from Mar 26, 2019
Merged

FIRApp: thread safety fixes #2639

merged 2 commits into from Mar 26, 2019

Conversation

@maksymmalyhin
Copy link
Contributor

@maksymmalyhin maksymmalyhin commented Mar 26, 2019

Fix #2624 :

  • use @synchronized(self) to fix thread safety.
@@ -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

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).

@@ -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

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

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

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

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.

@maksymmalyhin
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin commented Mar 26, 2019

@paulb777 I update the PR with @synchronized(self). As you suggested, I'll create a ticket to consider other ways to deal with concurrency in FIRApp

Copy link
Member

@paulb777 paulb777 left a comment

LGTM on travis green. Thanks!

BTW, standard practice is to put Fix #2624 in the PR's header description instead of title. Then GitHub will automatically close the associated issue when the PR merges.

@paulb777 paulb777 added this to the M46 milestone Mar 26, 2019
@maksymmalyhin maksymmalyhin changed the title FIRApp: thread safety fixes (#2624) FIRApp: thread safety fixes Mar 26, 2019
@maksymmalyhin maksymmalyhin merged commit 67c35d2 into master Mar 26, 2019
2 checks passed
@maksymmalyhin maksymmalyhin deleted the mm/firebaseUserAgent branch Mar 26, 2019
@toohotz
Copy link

@toohotz toohotz commented Apr 4, 2019

Glad this was fixed as it was causing an exception in my unit testing where ether MainThreadChecker was catching this. Thanks again @maksymmalyhin 👍🏾

@maksymmalyhin
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin commented Apr 4, 2019

@toohotz We are happy to help!

@toohotz
Copy link

@toohotz toohotz commented Apr 4, 2019

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 @syncrhonized(self) code) and I'm still having the main thread checking trap on my unit tests. This also happens on normal runtime but the thread checker allows the app to run but causes a runtime exceptionalities in the unit test (device only, sims works fine I assume due to the config not happening there)

Screenshot for reference

Imgur

@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?

@maksymmalyhin
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin commented Apr 4, 2019

@toohotz I cannot be 100% sure from the information provided, but it doesn't look like Firebase is involved here. I looks like [UIApplication applicationState] is accessed from a CoreMotion worker thread which triggers the Main thread checker exception. I am not aware of any part of Firebase using CoreMotion. Could you double check your application code, please?

@toohotz
Copy link

@toohotz toohotz commented Apr 4, 2019

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

Corrob added a commit that referenced this issue Apr 25, 2019
@firebase firebase locked and limited conversation to collaborators Oct 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants