Dedupe token renewals & pause outgoing requests while the token is renewed

[simonflk]

The problem

It's currently possible to get into situations where multiple renewToken calls are invoked, or calls which are certain to fail with a 401 will execute anyway and trigger another renew & retry.

Scenario 1:

1. 2 requests simultaneously execute with a token which turns out to be expired
2. Each will kick off its own renewToken()

Scenario 2:

1. 1 request executes with a token that turns out to be expired
2. renewToken() is executed
3. While the token is renewing, another request is made with the same expired token

What's been done

• Only invoke renewToken once per expired token
• If another request is made while a renewal is in progress, wait for the renewal to complete

Notes

The plugin maintains 2 bits of state:

1. The promise for the current/most recent renewToken
2. The expired token which triggered the renewToken call

I'm interested to hear what you think - I'm happy to make changes.

[ecyrbe]

Thank you for the improved version. I would like the shared state to be a little easier to understand and maintain.
using the token as a state for understanding if a token is currently being renewed is hard to follow.

Here is another idea, can you try and check if this works :

export function pluginToken(provider: TokenProvider): ZodiosPlugin {
  let pendingRenew: Promise<void> | undefined;
  let isRenewPending = false;

  return {
    request: async (_, config) => {
      if(isRenewPending) {
        await pendingRenew;
      }
      const token = await provider.getToken();
      if (token) {
        return {
          ...config,
          headers: {
            ...config.headers,
            Authorization: `Bearer ${token}`,
          },
        };
      }
      return config;
    },
    error: provider.renewToken
      ? async (_, __, error) => {
          if (
            axios.isAxiosError(error) &&
            provider.renewToken &&
            error.config
          ) {
            if (error.response?.status === 401) {
              let newToken: string | undefined = undefined;
              if (!isRenewPending) {
                pendingRenew = new Promise((resolve,reject)=> {
                  isRenewPending = true;
                  provider.renewToken().then((token) => {
                    newToken = token;
                    isRenewPending = false;
                    resolve();
                  },reject);
               }
              }
              await pendingRenew;

              if (isTokenRenewed(newToken, error)) {
                const retryConfig = { ...error.config };
                // @ts-ignore
                retryConfig.headers = {
                  ...retryConfig.headers,
                  Authorization: `Bearer ${newToken}`,
                };
                // retry with new token and without interceptors
                return axios(retryConfig);
              }
            }
          }
          throw error;
        }
      : undefined,
  };
}

[simonflk]

@ecyrbe I totally understand. Tbh, keeping the token as state was a fallback after trying several other solutions including something similar to what you propose here.

This works fine for this scenario:

• A request is initiated with a stale token
• Server responds with a 401
• Initiate renewToken()
• Another request is initiated

This solution allows us to pause the 2nd request until the token is renewed instead of initiating a 2nd renewToken.

But it only offers a partial solution to the other scenario which is quite common when paired with react-query's refetchOnWindowFocus:

• Multiple requests are initiated with a stale token
• The first request that gets a 401 response will kick off a renewToken()
• The other requests also return a 401

In this scenario, as long as all of those 401s come back before renewToken() resolves, we're ok - they will all wait for renewToken(), but if any of them take longer they will initiate further renewTokens()

How big a deal is this? This might be extremely edge-casey... Probably most servers will respond to a 401 from an expired token pretty quickly and most/all will come back before the token has been renewed. Is it worth the extra complexity, or can we accept that in some cases it will trigger multiple token renewals?

I'm happy to take a partial solution for now and think about if there's a more elegant way to handle this other scenario.

Thanks

[simonflk]

* There is a small bug there that the subsequent 401s which await pendingRenew will have newToken = undefined since the promise does not resolve with the new token, and the token which is updated in the Promise constructor is scoped to the initiating request.

We either need to resolve(token) or call getToken() after await pendingRenew

[simonflk]

I've updated the PR to exclude that edge case and in doing so we can simplify it to just one piece of state

[ecyrbe]

thanks for the update. here are my new comments.

[ecyrbe]

Why rethrow another error and hide the original ?
can you use ZodiosError (with a cause) so this way we at least retrow with a 'cause" that we can inspect.

[simonflk]

I'll try with the ZodiosError 👍🏼

I was just thinking that throwing axios.Cancel would let you test the error with axios.isCancel. And in this case we're not making a request, we're cancelling it.

[ecyrbe]

Don't this reset the pending promise token before the user get it back? Then below, newToken will be undefined in some cases?

[simonflk]

I don't think so, not in its current form, because clearing this reference will only affect instances which are not already await-ing the promise.

It would be an issue if there were some other async call before awaiting the renew token. e.g.

if (pendingRenew) {
  await doSomethingElse();
  const newToken = await pendingRenew; // now `pendingRenew` could be undefined
}

But your point is a good one. This does look a little inception-y and it seems like TS doesn't give us good hints in this case. Maybe it's worth keeping the isRenewPending for clarity's sake

[ecyrbe]

But it only offers a partial solution to the other scenario which is quite common when paired with react-query's refetchOnWindowFocus:

• Multiple requests are initiated with a stale token
• The first request that gets a 401 response will kick off a renewToken()
• The other requests also return a 401

In this scenario, as long as all of those 401s come back before renewToken() resolves, we're ok - they will all wait for renewToken(), but if any of them take longer they will initiate further renewTokens()

How big a deal is this? This might be extremely edge-casey... Probably most servers will respond to a 401 from an expired token pretty quickly and most/all will come back before the token has been renewed. Is it worth the extra complexity, or can we accept that in some cases it will trigger multiple token renewals?

I'm happy to take a partial solution for now and think about if there's a more elegant way to handle this other scenario.

You are right, the issue being that we don't know yet when we refetch on focus if the stale token request will fail. So we will have multiple request pending with 401. Let me try something on my side. I'll come back to you as soon as i can.

In the mean time are you blocked or can you keep your temporary solution in your codebase and switch to the released one later ?

[simonflk]

You are right, the issue being that we don't know yet when we refetch on focus if the stale token request will fail. So we will have multiple request pending with 401. Let me try something on my side. I'll come back to you as soon as i can.

In the mean time are you blocked or can you keep your temporary solution in your codebase and switch to the released one later ?

Thanks 🙏🏼

No problem at all, I have my version in our codebase for now, but looking forward to removing it when possible and using your version.

[ecyrbe]

@simonflk So after thinking about the second use case, maybe we can improve the plugin with an optional function to check if the token is expired ?
if your token is a JWT, or if you have a way to check if it the token has expired, we can prevent having to call the function and wait for renewal.

export type TokenProvider = {
  getToken: () => Promise<string | undefined>;
  renewToken?: () => Promise<string | undefined>;
  isTokenExpired?: () => boolean;
};

Tell me if that would be doable for you to provide a isTokenExpired function.
if so, i can provide a proper implementation.

[simonflk]

This would work for me. It's not a 100% solution because client & server might disagree on the time or the server might have revoked the token so might end up making the request and then the renewal anyway.

But that's no worse than now, and for sure an improvement.

So in that second case it would work something like this:

• Multiple requests are triggered on focus
• the first request synchronously checks if the token is expired
• if it is, it starts the renewal, and the other requests queue up behind it like in the first scenario

That sounds pretty good. Only downside is that the isTokenExpired() is synchronous. It's a bit strange given that getToken() is async. But that probably works for me. On initial load I'm pulling the token from an async storage, but after that it's always in memory.

[simonflk]

Actually, now that I think about it, I wonder if I can do all of this myself in userland...

declare function isValidToken(token: string | undefined): Promise<boolean>;
declare function getRefreshToken(): Promise<string>;
declare function getTokenFromStorage(): Promise<string | undefined>;
declare function getFreshToken(): Promise<string | undefined>;

const provider = (function () {
  let isRenewing = false;
  let pendingRenew: Promise<string | undefined> | undefined = undefined;

  return {
    async getToken() {
      const token = await getTokenFromStorage();
      if ((await isValidToken(token)) && !isRenewing) {
        return token;
      }
      return this.renewToken();
    },
    async renewToken() {
      if (isRenewing) {
        return pendingRenew;
      }

      isRenewing = true;
      pendingRenew = new Promise(async (resolve, reject) => {
        const token = await getFreshToken();
        resolve(token);
        isRenewing = false;
      });

      return pendingRenew;
    },
  };
})();

Not 100% sure this is a great idea, but it's nice to know that the API is flexible enough to allow people to add in their own logic.