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

fix: token refresh retry offline + recover on visible #278

Merged
merged 10 commits into from
May 6, 2022

Conversation

thorwebdev
Copy link
Member

src/lib/constants.ts Outdated Show resolved Hide resolved
src/GoTrueClient.ts Outdated Show resolved Hide resolved
src/lib/constants.ts Outdated Show resolved Hide resolved
@@ -703,7 +718,7 @@ export default class GoTrueClient {
if (expiresAt) {
const timeNow = Math.round(Date.now() / 1000)
const expiresIn = expiresAt - timeNow
const refreshDurationBeforeExpires = expiresIn > 60 ? 60 : 0.5
const refreshDurationBeforeExpires = expiresIn > EXPIRY_MARGIN ? EXPIRY_MARGIN : 0.5
this._startAutoRefreshToken((expiresIn - refreshDurationBeforeExpires) * 1000)
}

Copy link

@GaryAustin1 GaryAustin1 May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit:
It is not clear to me why you can not just call for a new refresh token if within EXPIRY_MARGIN+1 (+1 to not have to deal with fixing rounding and margin issues discussed below) and either way set the timer to expiresIn- EXPIRY_MARGIN. I think other changes require a minimum token expire length > EXPIRY_MARGIN by at least a few seconds to be useful, so the reason for the .5 (which was token life of less than 60 seconds) goes away.
::

I have an open issue that addresses in the first part .5 seconds as not enough time to complete refresh.
#272

I have a trace showing failure to do preflight and refresh request within 500msec in that issue (even though who ever came up with it felt it was enough time)

So if it is possible for the time left to expiration is EXPIRY_MARGIN + .5 or 1 or less, the token could expire for a brief amount of time if code can run and use the token before this task is finished (which I have not traced thru entirely).

I also think the rounding error mentioned in that issue still exists here and by cutting it so close with the .5 as Date.now() could get rounded up adding up .5 seconds you don't have if on the margin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've made the following decisions:

  1. We will default JWT expiry to 15mins in the future (once we've verified all the token refresh issues have been resolved)
  2. We will allow a minimum JWT expiry of 30s.
  3. We will default the expiry margin to 10s.
  4. In case of network failure we will retry every second.

So if you set JWT to the smallest possible value (30s), the client library will refresh it every 20s.

@thorwebdev thorwebdev requested a review from inian May 5, 2022 07:34
src/lib/fetch.ts Outdated
.catch((error) => handleError(error, reject))
.catch(() =>
reject({
message: NETWORK_FAILURE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are throwing even if result.ok is false, lets have a generic message like "Request Failed"? Cos this will get triggered both for network errors and server errors like timeouts etc

const { currentSession, expiresAt } = data
const timeNow = Math.round(Date.now() / 1000)

if (expiresAt < timeNow) {
if (expiresAt < timeNow + EXPIRY_MARGIN) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a counter and stop pinging gotrue or stop pinging after expiry time is reached cos gotrue is not going to refresh that token anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added exponential backoff, starting with 200ms and then exponentially backing off for 10 retries

@thorwebdev thorwebdev merged commit 8c6373b into master May 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2022

🎉 This PR is included in version 1.22.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

@thorwebdev thorwebdev deleted the fix/token-refresh-retry-offline+recover-on-visible branch May 9, 2022 02:21
@GaryAustin1
Copy link

GaryAustin1 commented May 9, 2022

@thorwebdev I can confirm the new supabase.js 1.35.3 refreshes the token when coming back from sleep in my test suite from supabase/supabase#6464

I do notice one issue though so far. My test code has a visibility handler to generate a select when tab goes visible. That fails with JWT expired. I put a delay of 5 seconds in to the select and it works. I put a delay of 100msec in and it did not work (still get JWT expired)

So at a minimum anyone using a visibility handler needs to check for token refreshed before executing an SB RLS call.

Worse case is though that ANY CODE THAT RUNS before the refresh finishes will error.

I will try and work on a test for this not using a visibility handler in my code, but I am starting a 10 day road trip in the morning, so may not be able to get to it quickly.

Select is in doStuff.

        if (document.visibilityState === "visible" ) {
            //await checkSession()
        setTimeout(function(){ doStuff(); }, 100);
       }

image

@GaryAustin1
Copy link

@thorwebdev If there is a better place for me to document findings on this pull request let me know.

I've not been able to figure out how to test the issue above without a visibility handler yet.

The following trace bothers me, and although I guess not a bug is certainly different in how gotrue.js will run.
I expected the new code to not do anything if the token was not within expire time, but it generates a signin event on every hidden to visible transition, which on a desktop tab could be 100's of times while the app is in use in an hour. On the desktop the refresh also normally occurs in background, so you should never have a stale token. Why even issue a signIn in event in the visibility handler, if token is good to go? Note: the visibility handler messaged below is mine, not gotrue's, but occur on same event.

tokenrefreshdesktop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants