fix: refresh token race condition #274

thorwebdev · 2022-04-21T09:35:46Z

What kind of change does this PR introduce?

Prevent token refresh race conditions and trigger recoverAndRefresh on visibility change.

What is the current behavior?

Currently our syncing across multiple tabs causes the tabs to race with each other because the setTimeout is set at exactly the same time when the user logs in in any of the tabs, as it is synced to the other tabs via local storage. All tabs then make the refreshAccessToken call at the same time, causing them to be invalidated.

Related issues: #213 supabase/supabase#6464 supabase/supabase#5990

What is the new behavior?

The new strategy is:

add a localStorage lock check to tell other tabs that a refresh request is in progress
add a 0-500ms random offset to the timeout to prevent most race conditions
use visibilitychange event to clearTimeout and restart autorefresh

Additional context

cc @GaryAustin1 @bnjmnt4n

thorwebdev · 2022-04-21T09:42:18Z

src/GoTrueClient.ts

@@ -643,7 +644,7 @@ export default class GoTrueClient {
      const { currentSession, expiresAt } = data
      const timeNow = Math.round(Date.now() / 1000)

-      if (expiresAt < timeNow) {
+      if (expiresAt < timeNow + 10) {


Maybe this should be 60s to give you more buffer? However when you set JWT expiry to 30s for example it then refreshes constantly. @kangmingtay maybe we should enforce a minimum expiry of 120 seconds?

I've changed it to 60 seconds to be consistent with the setTimeout refreshDurationBeforeExpires. However this means if your JWT expiry is below 60 seconds you're constantly calling gotrue. So we should probably enforce a minimum JWT expiry?

liaujianjie · 2022-04-21T10:08:40Z

Awesome stuff!! Does change this handle the case where a tab is closed mid-refresh? A lock could be acquired and not released.

thorwebdev · 2022-04-21T10:44:09Z

@liaujianjie it does now, thanks for pointing it out! 💚

inian

I think the flow is still a bit more complex than it needs to be, lets discuss offline

inian · 2022-04-21T12:20:43Z

package-lock.json

@@ -1,8 +1,7905 @@
 {
  "name": "@supabase/gotrue-js",
  "version": "0.0.0",
-  "lockfileVersion": 1,


remove this change Thor? Lock files should be v1 for now

inian · 2022-04-21T12:34:32Z

src/GoTrueClient.ts

@@ -634,7 +635,7 @@ export default class GoTrueClient {
   */
  private async _recoverAndRefresh() {
    try {
-      const json = isBrowser() && (await this.localStorage.getItem(STORAGE_KEY))
+      const json = isBrowser() && (await this.localStorage?.getItem(STORAGE_KEY))


extract localstorage.(get, set, removeitem) into a utility cos we seem to be doing isbrowser() everytime before accessing it and will lead to errors if we dont do that guard. (can be in a separate PR)

GaryAustin1 · 2022-04-21T13:29:47Z

@thorwebdev I just took a quick glance at the visibility handler. You probably do not want to automatically refresh on every visible event. You should do the expiration test for what ever time you agree to before renewing.

That event fires for any tab change on a browser. Desktop/mobile does not matter.

If the user tabs to check the stock market or news constantly, getting a new token will happen every time.
I know in my environment I've got 15 tabs open and move between 5 of them constantly during the day.

EDIT: adding. Also I don't think you want to shut of refreshing when going hidden, I think that is what you are doing. That would be a change to existing behavior of apps being able to run the the background on many desktops. Token refresh still occurs currently. Also this would break realtime in desktop background tabs. I keep realtime alive for 3 minutes when a tab goes to the background with a timer so people can tab around a bit without needed to reload from the database all the monitored data. The 3 minute limit is because of this: supabase/realtime-js#121

GaryAustin1 · 2022-04-21T14:48:28Z

If you are attempting to fix gotrue.js token refresh in general (versus just token race condition), I believe this issue of token refresh fails if the device is offline #226 (they also have a PR that seems overly complex to me) might need be addressed as well. I've not researched the online event handler. Also wondering what happens if the server/internet connection is down right at moment of token refresh timer going off and then comes back.

kangmingtay · 2022-04-21T22:06:45Z

src/GoTrueClient.ts

+              this.localStorage?.removeItem(`${STORAGE_KEY}-tokenRefreshLock`)
+              this._recoverAndRefresh()
+            },
+            5 * 1000 // remove lock after 5s


would it help if we randomise this setTimeout duration too? in the event there are multiple tabs waiting for the lock, there could be multiple instances of setTimeout invoked at the same time?

maybe it's better if the original lock setter calls setTimeout to remove the lock after 5 seconds after it acquires the lock here

Edit: I see the timer is for a release of the lock if the "other" tabs does not unlock, which covers below. Sorry for the interruption...

On the lock with a timer... What happens if the tab that set the lock gets closed right after that?
I'll admit I'm not up to speed on the need for multiple tabs and what the user can do with these tabs, but they can certainly close one at any point.

ok i just tested out the following and closed the tab before the timeout expired:

setTimeout(function(){ console.log("Hello from setTimeout") }, 5000);

seems like once you close the tab, the timeout gets cleared so yeah it needs to be called by another tab and not the original lock setter

thorwebdev · 2022-04-26T15:28:09Z

I switched course and decided to not use the lock but rather rely on a combination of

a) random timeout offset: https://github.com/supabase/gotrue-js/pull/274/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5R716
b) check if the token is still valid within the margin and in that case not refresh: https://github.com/supabase/gotrue-js/pull/274/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5R683

It does limit the chance of the race condition happening, but it does feel a bit hacky... Would appreciate your thoughts!

GaryAustin1 · 2022-04-27T03:52:52Z

I switched course and decided to not use the lock but rather rely on a combination of

a) random timeout offset: https://github.com/supabase/gotrue-js/pull/274/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5R716 b) check if the token is still valid within the margin and in that case not refresh: https://github.com/supabase/gotrue-js/pull/274/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5R683

It does limit the chance of the race condition happening, but it does feel a bit hacky... Would appreciate your thoughts!

Upfront I'll say I'm lost on where this is as far as the "final" code as I'm new to github tracking...

I'm out of time to keep looking tonight, but my scan of what I think the latest GoTrueClient.ts has dropped the visibility event handler. If I understand correctly this PR was to fix some race condition, but then several issues with token expiring while tab is not in focus or in sleep mode got "pulled in". The visibility handler code was a key part of that possible solution. I think that is gone now, but as I said, this is a maze for me at the moment...

My view is that the Supabase client should take care of refreshing the jwt token in all cases where the refresh token is valid without user dev interaction. This can be done right before every authorization header set up in supabase.js, on "token expired" error with a retry on the actual database request, or with visibility handler to deal with dead timer when device MAY have been sleeping (you can not assume "hidden" state means timers have stopped).

IMO the refresh token timer should be an optimistic event to save time on an actual database call or realtime status, BUT should NOT be the final solution. Supabase.js and realtime.js should on their own be responsible for verifying token about to expire before they make a request and attempt to refresh it or deal with error gracefully (not easy for realtime).

At a minimum the visibility handler is needed to deal with dead timer on mobile (and apparently other situations) when the tab does not have focus or device is asleep and token expires.

Once again sorry, it is late but wanted to to throw this out.

thorwebdev · 2022-04-27T05:44:16Z

Thanks, Gary, I’m also not happy with this PR. Agreed, I think there’s a couple of things we need to fix

The race condition needs to be handled server-side. There is just too many edge cases, including network request issues, that just can’t be handled on the client (cc @kangmingtay)
The visibility handler to recover the session when the window becomes visible.
The retries for failed refresh token requests

thorwebdev · 2022-05-04T04:24:54Z

is being addressed server-side here: fix: add reuse interval for token refresh auth#466
& 3. are being addressed here: fix: token refresh retry offline + recover on visible #278

thorwebdev added 2 commits April 21, 2022 17:22

fix: refresh token race condition

6acf804

chore: remove console.logs

2232eb7

thorwebdev requested review from silentworks, inian and kangmingtay April 21, 2022 09:35

thorwebdev commented Apr 21, 2022

View reviewed changes

thorwebdev requested review from kiwicopple and awalias April 21, 2022 09:42

thorwebdev linked an issue Apr 21, 2022 that may be closed by this pull request

Concurrent token refreshes can lead to unintentional signing out of users when multiple tabs are open #213

Closed

thorwebdev mentioned this pull request Apr 21, 2022

The users JWT expires if on same page longer than expire time. supabase/auth-helpers#73

Closed

fix: add lock timeout

a471a16

thorwebdev mentioned this pull request Apr 21, 2022

Automatically refresh access token before it expires supabase/auth-helpers#78

Closed

thorwebdev linked an issue Apr 21, 2022 that may be closed by this pull request

Token refresh time is likely set too "short" if less than 60 seconds left before it expires. #272

Closed

inian reviewed Apr 21, 2022

View reviewed changes

kangmingtay reviewed Apr 21, 2022

View reviewed changes

kiwicopple mentioned this pull request Apr 25, 2022

Not possible to keep user signed in #323

Closed

thorwebdev added 5 commits April 26, 2022 17:49

fix: localStorage sync and retry requests

ab55838

fix: check token expiration

32ff61e

fix: remove lock for random offset

0c94e0a

chore: add note

f232c46

fix: refreshSession

d99d71c

thorwebdev linked an issue Apr 26, 2022 that may be closed by this pull request

Token refresh fails silently when offline and never recovers #226

Closed

thorwebdev marked this pull request as draft April 27, 2022 05:45

kangmingtay mentioned this pull request May 3, 2022

fix: add reuse interval for token refresh supabase/auth#466

Merged

thorwebdev closed this May 4, 2022

thorwebdev deleted the fix/refresh-race-condition branch May 25, 2022 14:11

GaryAustin1 mentioned this pull request Jun 11, 2022

onAuthStateChange (SIGNED_IN event) Fired everytime I change Chrome Tab or refocus on tab . supabase/supabase#7250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refresh token race condition #274

fix: refresh token race condition #274

thorwebdev commented Apr 21, 2022

thorwebdev Apr 21, 2022

thorwebdev Apr 21, 2022

liaujianjie commented Apr 21, 2022

thorwebdev commented Apr 21, 2022

inian left a comment

inian Apr 21, 2022

inian Apr 21, 2022

GaryAustin1 commented Apr 21, 2022 •

edited

Loading

GaryAustin1 commented Apr 21, 2022 •

edited

Loading

kangmingtay Apr 21, 2022

GaryAustin1 Apr 24, 2022 •

edited

Loading

kangmingtay Apr 25, 2022 •

edited

Loading

thorwebdev commented Apr 26, 2022

GaryAustin1 commented Apr 27, 2022 •

edited

Loading

thorwebdev commented Apr 27, 2022

thorwebdev commented May 4, 2022

fix: refresh token race condition #274

fix: refresh token race condition #274

Conversation

thorwebdev commented Apr 21, 2022

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Additional context

thorwebdev Apr 21, 2022

Choose a reason for hiding this comment

thorwebdev Apr 21, 2022

Choose a reason for hiding this comment

liaujianjie commented Apr 21, 2022

thorwebdev commented Apr 21, 2022

inian left a comment

Choose a reason for hiding this comment

inian Apr 21, 2022

Choose a reason for hiding this comment

inian Apr 21, 2022

Choose a reason for hiding this comment

GaryAustin1 commented Apr 21, 2022 • edited Loading

GaryAustin1 commented Apr 21, 2022 • edited Loading

kangmingtay Apr 21, 2022

Choose a reason for hiding this comment

GaryAustin1 Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

kangmingtay Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

thorwebdev commented Apr 26, 2022

GaryAustin1 commented Apr 27, 2022 • edited Loading

thorwebdev commented Apr 27, 2022

thorwebdev commented May 4, 2022

GaryAustin1 commented Apr 21, 2022 •

edited

Loading

GaryAustin1 commented Apr 21, 2022 •

edited

Loading

GaryAustin1 Apr 24, 2022 •

edited

Loading

kangmingtay Apr 25, 2022 •

edited

Loading

GaryAustin1 commented Apr 27, 2022 •

edited

Loading