Do not use recursive getToken() loop #3260
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/gettokenloop
branch
from
d7b77c1 to
d1216e3
Compare
Binary Size ReportAffected SDKs
Test Logs |
🦋 Changeset is good to goLatest commit: 71e52aa We got this. This PR includes changesets to release 10 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 |
|
FWIW, the newest user report is from a user that doesn't use Firestore. |
| // user, we can't be sure). | ||
| if (this.tokenCounter !== initialTokenCounter) { | ||
| if (this.lastToken) { | ||
| // If we received a new token while this callback was pending (via |
There was a problem hiding this comment.
Question: what is the reason to prefer lastToken to tokenData here?
There was a problem hiding this comment.
I would like you to verify this - but I think this matches the previous behavior. If we receive a token update via our callback listener, we ignore the token that is returned from the getToken call.
FWIW, while I think this is the cleaner solution to our token handling, I think the risk of it probably means we should not merge it. After all, we now have evidence that the problem we are trying to solve also exists in at least one other place (and maybe only that place).
Maybe @wilhuff wants a distraction from his current work and take a peak as well.
There was a problem hiding this comment.
The reason this is complicated is that we need to subscribe to auth to get notified when the identity changes, so that we can trigger user changes in the local store.
The reason we ignored the token in that callback was because when remote store wants to restart streams it it's going to trigger a call to getToken. This restart might happen significantly later, because there may not have been active streams when the identity changed.
The central question we need to answer is whether or not the token listener will call us before the current access token expires. If it does, it seems like we could just always use the token supplied by the token listener, no? If we did that this could be much simpler.
|
Closing this for now. |
There has been another user affected by #3222, so I am sending this out for review now. This reduced the change of a possible
getTokenloop if our token is invalid by just using the last received token.Fixes #3222