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

Handle Concurrent API Auth Failures #515

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Conversation

sternetj
Copy link
Contributor

Changes

  • Adds a promise ref to track ongoing refreshes and prevent calling refresh concurrently

Reasoning

  • I noticed that the app logs you out if you make two immediate calls to refreshForAuthFailure. This is because the refresh token can only be used once; so the first refresh succeeded but the second one received invalid_grant and caused the app to log out.

Notes

  • There was already some logic to handle this scenario but it used the loading state value which I think was lagging behind when two requests failed simultaneously. This uses a ref so it shouldn't lag behind because I ensured the logic is synchronous up to the point of where the ref value is set.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7507674600

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 84.994%

Totals Coverage Status
Change from base Build 7489673850: 0.03%
Covered Lines: 3608
Relevant Lines: 4129

💛 - Coveralls

Copy link
Member

@aecorredor aecorredor left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Were we able to validate it does work?

Copy link
Contributor

@swain swain Jan 12, 2024

Choose a reason for hiding this comment

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

Confirming: the idea here is that we just don't want two concurrent calls to refreshAuthToken, right?

If that's right, I think we could have a simpler implementation by just https://www.npmjs.com/package/p-limit

import pLimit from 'p-limit';

const limited = pLimit(1);

// ...

await limited(async () => {
  const refreshResult = await _refreshHandler(_authResult);
  await storeAuthResult({
    ...refreshResult,
    // Careful not to lose `refreshToken`, which may not be in `refreshResult`
    refreshToken: refreshResult.refreshToken || _authResult.refreshToken,
  });

  // ...
})

I think that'd allow us to avoid refs, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is what we want. If I am understanding p-limit correctly, N calls to refresh with still result in N refresh attempts. While this would probably work. It seems wasteful to make unneeded refresh calls if the first one succeeds. Similarly, if the first one fails, the others will also fail.

The code I have right now caches the first promise and returns it to all subsequent calls until the first promise finishes.

Is there a p-X library that would do that? p-throttle looks close but the time requirement doesn't match with my use-case.

Example: I want to achieve all 1's since it is the first call but this prints 1, 2, 3
import pLimit from 'p-limit';

const limit = pLimit(1);

const doIt = (a) =>
  limit(() => new Promise((done: any) => setTimeout(done(a), 5000)));

doIt(1).then(console.log);
doIt(2).then(console.log);
doIt(3).then(console.log);

@sternetj
Copy link
Contributor Author

Makes sense to me. Were we able to validate it does work?

  • I added the test case to verify
  • Added a button locally to trigger two immediate refreshes to verify
  • Added some logic locally to the httpClient to simulate a 401 for multiple calls and it refreshed it correctly

Copy link
Contributor

@markdlv markdlv left a comment

Choose a reason for hiding this comment

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

Beautifully simple solution

@sternetj sternetj merged commit f6130bb into master Jan 15, 2024
3 checks passed
@sternetj sternetj deleted the handle-concurrent-401s branch January 15, 2024 22:08
Copy link

🎉 This PR is included in version 11.11.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Has been released to the package repository (NPM, etc) label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Has been released to the package repository (NPM, etc)
Projects
None yet
5 participants