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.

Sign up for GitHub

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

Jump to bottom

Handle Concurrent API Auth Failures #515

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Handle Concurrent API Auth Failures #515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/hooks/useAuth.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ test('refreshForAuthFailure refreshes auth token if not already loading', async
jest.useFakeTimers();
act(() => {
result.current.refreshForAuthFailure(new Error() as AxiosError);
result.current.refreshForAuthFailure(new Error() as AxiosError);
});
expect(refreshHandler).toHaveBeenCalledTimes(1);
expect(result.current.loading).toBe(true);
Expand Down
46 changes: 31 additions & 15 deletions src/hooks/useAuth.tsx
View file
Open in desktop
Copy link
Contributor

@swain swain Jan 12, 2024 •
edited
Loading

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);

Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React, {
useCallback,
useContext,
useEffect,
useRef,
useState,
} from 'react';
import { RefreshResult } from 'react-native-app-auth';
Expand Down Expand Up @@ -70,6 +71,7 @@ export const AuthContextProvider = ({
RefreshHandler | undefined
>();
const { currentAppState } = useCurrentAppState();
const refreshLock = useRef<Promise<void>>();

const storeAuthResult = useCallback(async (result: AuthResult) => {
await secureStorage.setObject(result);
Expand All @@ -87,23 +89,37 @@ export const AuthContextProvider = ({

const refreshAuthResult = useCallback(
async (_refreshHandler: RefreshHandler, _authResult: AuthResult) => {
if (__DEV__ && process.env.NODE_ENV !== 'test') {
console.warn('Attempting access token refresh');
const refreshSession = async () => {
if (__DEV__ && process.env.NODE_ENV !== 'test') {
console.log('Attempting access token refresh');
}
try {
setLoading(true);
const refreshResult = await _refreshHandler(_authResult);
await storeAuthResult({
...refreshResult,

// Careful not to lose `refreshToken`, which may not be in `refreshResult`
refreshToken:
refreshResult.refreshToken || _authResult.refreshToken,
});
} catch (error) {
if (process.env.NODE_ENV !== 'test') {
console.warn('Error occurred refreshing access token', error);
}
clearAuthResult();
}
};

if (refreshLock.current) {
return refreshLock.current;
}

try {
setLoading(true);
const refreshResult = await _refreshHandler(_authResult);
await storeAuthResult({
...refreshResult,

// Careful not to lose `refreshToken`, which may not be in `refreshResult`
refreshToken: refreshResult.refreshToken || _authResult.refreshToken,
});
} catch (error) {
if (process.env.NODE_ENV !== 'test') {
console.warn('Error occurred refreshing access token', error);
}
clearAuthResult();
refreshLock.current = refreshSession();
await refreshLock.current;
} finally {
refreshLock.current = undefined;
}
},
[storeAuthResult, clearAuthResult],
Expand Down
Loading