Repository navigation
Conversation
| /// The user's current tokens. Read once - accessing individual properties in sequence is not | ||
| /// recommended. Reading once ensures that each property is from the same update. Writes are | ||
| /// serialized by `@synchronized(self)`. | ||
| @property(atomic, strong, nullable) GIDGoogleUserTokens *tokens; |
There was a problem hiding this comment.
I think atomic is the default. Happy to leave as-is if you think explicitness is useful.
| - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion { | ||
| if (!([self.accessToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire || | ||
| (self.idToken && [self.idToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire))) { | ||
| // A single read here avoids accidnetally reading tokens from multiple snapshots. |
There was a problem hiding this comment.
s/accidnetally/accidentally
| } | ||
| if ((self.idToken || idToken) && ![self.idToken isEqualToToken:idToken]) { | ||
| self.idToken = idToken; | ||
| @synchronized(self) { |
There was a problem hiding this comment.
I think this lock fully covers the snapshot write, but I want to check something about the refresh path.
-updateWithTokenResponse:authorizationResponse:profileData: was already under @synchronized(self) before this change, so (as I understand it) the writer that actually had no lock is the refresh callback in -refreshTokensIfNeededWithCompletion:. It calls [self.authState updateWithTokenResponse:error:] (and updateWithAuthorizationError:) without holding the lock, and only gets here through -didChangeState:.
So the tokens write is now protected, but OIDAuthState itself can still change on that unlocked path while this method is building tokens from it (e.g. a sign-in or addScopes update running at the same time as a refresh). That problem existed before this PR, so I don't think it needs to block this one.
A couple of thoughts:
Could we mention this in the PR description, or file a follow-up issue, so it's clear the PR makes the token snapshot safe but doesn't make OIDAuthState thread-safe?
Would it be cheap to wrap the authState updates in the refresh callback in @synchronized(self) as well? -didChangeState: would then re-enter this lock, which is fine since it's recursive. Other code still reads authState without the lock (grantedScopes, encodeWithCoder:, and the fetcher authorizer), so fully fixing that is probably a follow-up either way.
Related to tests: both stress tests drive -updateWithTokenResponse:authorizationResponse:profileData:, which was already locked. A variant that races -didChangeState: (or [user.authState updateWithTokenResponse:error:]) against reads would cover the path this PR actually changes. Ideally use a different refresh token for each state so a mixed refresh token would be caught too.
Happy to take this as a follow-up if you'd prefer to keep this PR focused.
GIDGoogleUserwrote its access, refresh and ID tokens one property at a time - as a result, readers could see a mix of old & new tokens.Behaviour change (in the CHANGELOG):
accessToken,refreshToken,idTokenare now notified when any of the three changes. KVO observers run while the user's lock is held.The second behaviour change is an intentional tradeoff. The suggestion here was to send the KVO outside of the lock. That would mean out-of-order delivery could occur between any concurrent updates, and notifications be sent out-of-order. Sending outside of the lock would also mean that observers who ask KVO for the old value would get the new one. The only way for us to guarantee that there is no chance of deadlock; KVO freshness is correct; and notifications are delivered in-order would be to implement a manual notification queue.
Given that a deadlock would only occur when an observer is synchronously waiting on another thread that is also updating that same user; and that this behaviour has been present on the login path for some time; I judged that amount of manual machinery as not worth the tradeoff. Very willing to revisit in the future.