Conversation
🦋 Changeset detectedLatest commit: 15a0524 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Collaborator
Size Report 1Affected Products
Test Logs |
Collaborator
Size Analysis Report 1Affected Products
Test Logs |
Collaborator
Size Analysis Report 1Affected Products
Test Logs |
dwyfrequency
approved these changes
Sep 21, 2022
Contributor
dwyfrequency
left a comment
There was a problem hiding this comment.
Great work!! Everything looks good to me here
|
Hi @hsubox76 Do you know if this fix has been published yet? I'm currently running into mega infinite loop App Check problems (we're getting 90K invalid requests constantly every single hour for the last 7 days) and I think this may go a long way to fix them. We're really keen to update and try this fix as soon as possible. Thanks for all the hard work |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6373
Causes
Found 2 causes of the problems:
getToken()needs to return an error result (a dummy token with anerrorproperty) whenever there is an error. It was assuming that if there was an error there would be no token. This is not always the case. If the token is about to expire, the proactive refresh attempt may run into an error at the exchange endpoint. Then there will be an existing valid token, but also en error.getToken()makes a request to the exchange endpoint, it stores the promise instate.exchangeTokenPromiseand doesn't make another request if the promise is in flight, instead, any calls during that period await the existing promise. Once the promise is resolved, it clearsstate.exchangeTokenPromise, allowing future exchange requests to be made when needed. The mistake was that it only cleared this when the promise was resolved, and not when the promise was rejected, so that once an exchange request promise failed, another one wouldn't be made until memory was cleared. (This may be why users had to refresh the page.)Solutions
getToken()to return a token with a special error property,internalError. The reason this needs to be different fromerror, is that there's 2 different places that look for theerrorproperty on the result returned bygetToken(). The first is the Refresheroperationcallback (defined insidecreateTokenRefresher), where we need it to find theerrorproperty (indicating there was an exchange request error) and throw, which will cause the Refresher to go into backoff retry mode. However, the other place that looks aterrorisnotifyTokenListenerswhich calls the error callback for any 3P listeners if it findserror. If we have a valid token still, and it's just the proactive refresh attempt that's failing, we should just keep sending the token to 3P listeners as usual, it isn't an error from their point of view. That's why we useinternalErrorfor this case, sonotifyTokenListenerscan ignore it and the Refresher's operation callback can use it and throw, just as it does forerror.thenwithfinally. Thereturn tokenpart of thethencallback is no longer needed as the original promise already returns the token.Testing
Tested this using a 30 minute token and taking the tab offline using devtools, after initial token fetch. While offline, the app attempted to make requests to Firestore, and later App Check, after the App Check token had expired, but no more than 1 request per minute (and I think most were triggered by Firestore, which needs to get an App Check token when it tries to connect to the backend). After returning tab to online state, it immediately got a new token, and Firestore was able to write.