-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
fix: token refresh retry offline + recover on visible #278
Conversation
@@ -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) | |||
} | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- We will default JWT expiry to 15mins in the future (once we've verified all the token refresh issues have been resolved)
- We will allow a minimum JWT expiry of 30s.
- We will default the expiry margin to 10s.
- 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.
src/lib/fetch.ts
Outdated
.catch((error) => handleError(error, reject)) | ||
.catch(() => | ||
reject({ | ||
message: NETWORK_FAILURE, |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
🎉 This PR is included in version 1.22.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@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.
|
@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. |
What kind of change does this PR introduce?
In combination with supabase/auth#466 this PR should resolve the most critical token refresh issues,
cc @GaryAustin1