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: resolve & reset deferred upon refresh error #339

Merged
merged 8 commits into from
Aug 10, 2022

Conversation

pixtron
Copy link
Contributor

@pixtron pixtron commented Aug 1, 2022

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

Currently refreshingDeferred does not get resolved/rejected upon error. Also the deferred does not get reset to null during an error. See #334

What is the new behavior?

The deferred gets resolved upon AuthError and rejected upon other errors, and finally reset to null. Like this the deferred Promise behaves like the Promise returned by _callRefreshToken itself.

Additional context

Closes #334

pixtron added 2 commits August 1, 2022 09:41
If error is not an AuthError _callRefrehToken throws the error
This commit ads a test and fix for this scenario.
@@ -37,6 +37,14 @@ import type {

polyfillGlobalThis() // Make "globalThis" available

type CallRefreshTokenResult = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This type is solely used in private methods and properties. Not sure if it nevertheless should go to lib/types.ts? Both is fine for me.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's put this in lib/types.ts to keep things consistent 😄

): Promise<CallRefreshTokenResult> {
// refreshing is already in progress
if (this.refreshingDeferred) {
return this.refreshingDeferred.promise
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved that out of the try block and removed the await. Otherwise a rejection would fall into catch and finally multiple times. As _callRefreshToken throws non AuthErorr's that should be the expected behavior.

@@ -1,4 +1,5 @@
import { OpenIDConnectCredentials } from '../src'
import { AuthError } from '../src/lib/errors';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors are not exported by the library. Would it make sense to export them so a consumer can import errors and compare them with instanceof AuthError or isAuthError(error)?

Copy link
Member

Choose a reason for hiding this comment

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

Good point – we definitely should at least be exporting isAuthError()

@kangmingtay kangmingtay requested a review from alaister August 2, 2022 02:01
@pixtron pixtron changed the title Fix/issue 334 fix: resolve & reset deferred upon refresh error Aug 3, 2022
@wattroll
Copy link

wattroll commented Aug 8, 2022

Thank you for looking into this. This fixes the problem for me.

  • Coding wise, would it make sense to move the actual callRefreshToken logic into another private function (e.g. callRefreshTokenConcurrently) that will not be concerned with simultaneous calls? The callRefreshToken will then only be creating/resolving/rejecting the deferred promises as appropriate to share the concurrent calls. That is to make it easier to ensure that the caller will observe the same behaviour when they get a newly created promise and when they get an existing one. And also to keep things more robust against future changes. Making it less likely that somebody who touches error-matching logic, or session saving, or subscriber notifications will introduce another dangling promise bug.

  • Have you considered adressing the problem of this being called with different refresh tokens simultaneously? e.g. _callRefreshToken(token_a) and _callRefreshToken(token_b) should not resolve to the same result. I don't think this is exposed to the users now, at least not in any common path, but it's a potential issue.

For reference here is an earlier implementation of shared token refreshes, where these two points are addressed https://github.com/supabase/gotrue-js/pull/203/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5

@pixtron
Copy link
Contributor Author

pixtron commented Aug 8, 2022

Regarding @wattroll's points:

would it make sense to move the actual callRefreshToken logic into another private function (e.g. callRefreshTokenConcurrently) that will not be concerned with simultaneous calls. The callRefreshToken will then only be creating/resolving/rejecting the deferred promises as appropriate to share the concurrent calls.

The implementation in #203 does not protect from race condition. _callRefreshTokenConcurrently() could accidentally be called internally without going through _callRefreshToken() and like this introduce concurrent calls to this.api.refreshAccessToken(). Therefore I prefer @alaister's implementation introduced into next with #285, regarding this point.

Have you considered adressing the problem of this being called with different refresh tokens simultaneously? e.g. _callRefreshToken(token_a) and _callRefreshToken(token_b) should not resolve to the same result. I don't think this is exposed to the users now, at least not in any common path, but it's a potential issue.

This indeed is a potential issue. But the solution to have a promise/deferred for each token would shift the race condition down to saveSession(). When calling _callRefreshToken(token_a) and _callRefreshToken(token_b) concurrently the last one that resolves would write to the storage. As the storage in gotrue-js currently is a single user storage it probably would make more sense to abort pending refresh requests, when _callRefreshToken() is called concurrently with another refresh token than the pending request was called with. It should be possible to do that by passing an abort signal to the fetch request.

My intention for this PR is solely to fix a Bug (see #334) introduced with #285. Probably it makes sense to open a new Issue with @wattroll 's points to discuss them further?

Copy link
Member

@alaister alaister left a comment

Choose a reason for hiding this comment

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

Thanks, @pixtron! This looks great!

Would you mind running prettier (npm run format) on your code and moving CallRefreshTokenResult to lib/types.ts, then I'll merge this in.

Thanks for adding tests too!

@pixtron
Copy link
Contributor Author

pixtron commented Aug 9, 2022

Moved CallRefreshTokenResult to lib/types.ts and comited prettier changes affecting code introduced with this PR. I didn't dare to commit prettier changes to other areas of the code base.

Prettier changed this line

const { session: session3, error: error3 } = await authWithSession._callRefreshToken(session?.refresh_token)

to 3 lines, which resulted in the TS error Type 'undefined' is not assignable to type 'string' during tests. When the code is on one line this error is not thrown. I decided to use a non null assertion as there is already a expect(session).not.toBeNull() a couple of lines above.

@pixtron pixtron requested a review from alaister August 9, 2022 10:45
Copy link
Member

@alaister alaister left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes.
I agree that the non-null assertion is fine there.

Cheers!

@alaister alaister merged commit ae4f1de into supabase:next Aug 10, 2022
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.23.0-next.12 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.0.0-rc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants