Synchronize useUserAccessGroup #5387
Conversation
FirebaseAuth/Sources/Auth/FIRAuth.m
Outdated
| error:(NSError *_Nullable *_Nullable)outError { | ||
| // self.storedUserManager is initialized asynchronously. | ||
| __block FIRAuthStoredUserManager *storedUserManager; | ||
| dispatch_sync(FIRAuthGlobalWorkQueue(), ^{ |
There was a problem hiding this comment.
This creates the potential for deadlock because useUserAccessGroup:error: is called from code already on the FIRAuthGlobalWorkQueue()
See
There was a problem hiding this comment.
Thanks @wilhuff Good catch! PTAL at the update and let me know if you have a suggestion for a better way.
|
I'll see if I can add a unit test as well before merging. |
FirebaseAuth/Sources/Auth/FIRAuth.m
Outdated
| - (BOOL)useUserAccessGroup:(NSString *_Nullable)accessGroup | ||
| error:(NSError *_Nullable *_Nullable)outError { | ||
| // Make sure that the keychain has been initialized. | ||
| dispatch_semaphore_wait(_initializingKeychain, DISPATCH_TIME_FOREVER); |
There was a problem hiding this comment.
This approach of splitting internal and external usage would work just as well with your dispatch_sync approach and would avoid needing to keep an extra semaphore (or keep waiting/re-signaling it).
|
|
||
| - (BOOL)useUserAccessGroup:(NSString *_Nullable)accessGroup | ||
| error:(NSError *_Nullable *_Nullable)outError { | ||
| - (BOOL)internalUseUserAccessGroup:(NSString *_Nullable)accessGroup |
There was a problem hiding this comment.
Line 532 needs to change to use the internal version and you're done.
The test needs to set up a stored user access group to effectively verify that the deadlock during initialization isn't happening.
| - (BOOL)useUserAccessGroup:(NSString *_Nullable)accessGroup | ||
| error:(NSError *_Nullable *_Nullable)outError { | ||
| // self.storedUserManager is initialized asynchronously. Make sure it is done. | ||
| dispatch_sync(FIRAuthGlobalWorkQueue(), ^{ |
There was a problem hiding this comment.
I'm thinking maybe it makes sense to load storedUserManager synchronously, but it requires more investigation on the impact. This should be good for now.
Fix #4175