Dangling Promises in _callRefreshToken

[pixtron]

Bug report

Describe the bug

Upon an error the method _callRefreshToken does not resolve the refreshingDeferred, leading to dangling promises. If an error during a refresh token request occurs the deferred is kept for ever, and all subsequent calls to _callRefreshToken will await the deferred for an infinite time as it will never resolve.

To Reproduce

1. clone the repro
2. npm install
3. npm start
4. Set breakpoints on these lines:
   • if (this.refreshingDeferred)
   • const { session, error } = await this.api.refreshAccessToken(refresh_token)
5. Signup with email and password (or login if you already have an account)
6. Disable all network interfaces
7. Wait 2 minutes until token expires and auto refresh is triggered (there will be no retries Backoff upon error during auto refresh #333)
8. Enable network again
9. Sign out
10. Sign in again
11. Wait 2 minutes for refresh

The condition if (this.refreshingDeferred) will be true and this.refreshingDeferred from the former refresh request in step 7 is returned and never rejected nor resolved.

Expected behavior

The deferred should be resolved with the error result and then be reset to null.

Code sandbox

I created a sandbox to simulate the behavior.

Auth Error

1. Click two times on the Button "Refresh Auth Error"
2. Wait a second
3. We only get a result for request Init #1 NOK: we should get a result for req setting current session after successful third party login #2 as well
4. Click again on any Button
5. Request Init #1 is triggered, but never gets a result NOK

Generic Error

Same scenario as Auth Error but with Button "Refresh Generic Error", throws the error instead of resolving with an error response.

Success case works as expected

1. Click tow times on the Button "Refresh Success"
2. Wait a second
3. Both requests get resolved with the result of req Init #1 OK

Suggested Fix

1. Move this.refreshingDeferred = null into a finally block so it gets reset in success and error case.
2. If the error is an AuthError, resolve this.refreshingDeferred with the error result in the catch block.
3. If the error is not an AuthError, reject this.refreshingDeffered with the thrown error
4. Move return await this.refreshingDeferred.promise out of the try block and don't await it (return this.refreshingDeferred.promise). This is to avoid multiple entries into catch and finally block, when the error is not an AuthError and therefore re thrown (see 3).

I created a sandbox simulation with the suggested fix

System information

• OS: NA
• Browser: NA
• Version of supabase-js: NA
• Version of gotrue-js: 1.23.0-next.6
• Version of Node.js: NA

Additional Context

This regression seems to have been introduced in 1.23.0-next with #285.

[github-actions]

🎉 This issue has been resolved in version 1.23.0-next.12 🎉

The release is available on:

• GitHub release
• npm package (@next dist-tag)

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 2.0.0-rc.1 🎉

The release is available on:

• GitHub release
• npm package (@rc dist-tag)

Your semantic-release bot 📦🚀

[kangmingtay]

Closing this since it seems to have been resolved in the PR above, thanks @pixtron !