Closed parsley72 closed 1 year ago
This is imho a wrong implementation.
axios.timeout is not relevant for Error 429. Error 429 means, that the server has too many requests and in retry-after-header you get two types of responses. A datetime object or an delay in seconds
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
So a potential solution would be actually (it is typescript, so just remove it to have just javascript)
export async function handleRetryAfter(error: AxiosError, resolve: <T = any>(value?: T | PromiseLike<T>) => void): Promise<void> {
if (error.response) {
const retryAfter = error.response.headers["retry-after"];
let retryAfterInMs = 0;
if (/\d+/.test(retryAfter)) {
retryAfterInMs = retryAfter * 1000;
}
if (
typeof retryAfter === "string" &&
isNaN(new Date(retryAfter as string).getTime()) === false
) {
retryAfterInMs = new Date(retryAfter).getTime() - (error.config as AxiosRetryRequestConfig).lastRequestTime;
}
if (retryAfterInMs) {
await new Promise(resolveSleep => setTimeout(resolveSleep, retryAfterInMs));
}
}
resolve();
}
and in the response interceptor you should then change to
return new Promise(
resolve =>
new Promise(resolveRetryAfter => handleRetryAfter(error, resolveRetryAfter)
.then(() => setTimeout(() => resolve(axios(config)), delay))));
So you resolveRetryAfter after the corresponding retry and then you do the actual call.
What I needed was an error 429 handler which would use Retry-After but only to a point - the particular API I'm calling can return a Retry-After up to 15 mins long. I don't want my page to wait that long, I want to have a 5 second cutoff, so I need a mechanism to cancel the retry. Without that I don't need to change anything in axios-retry, you can just use https://github.com/softonic/axios-retry/issues/72#issuecomment-699785860.
You're right about handling the date case, I hadn't noticed that (since the API I'm using only returns the seconds case).
Then you have to actually not set a timeout but introduce another option e.g. maxRetryAfter, which handles the corresponding abort condition, when the retry would take too long.
Why? I don't think you're reading the PR properly - the new retryAfter()
function just checks for a Retry-After value. The implementation that includes a timing limit is just a comment that's provided as an example - you can change the value to whatever you want.
Yes you are right. I implemented it now differently. Thank you for your clarification
Updated with Date handling, minor fixes.
@Uzlopak thoughts?
Well like I wrote I implemented it differently.
I used your code as base and did it like this:
/**
* The Retry-After response HTTP header indicates how long the user agent
* should wait before making a follow-up request. There are three main cases
* this header is used:
*
* - When sent with a 503 (Service Unavailable) response, this indicates how
* long the service is expected to be unavailable.
* - When sent with a 429 (Too Many Requests) response, this indicates how
* long to wait before making a new request.
* - When sent with a redirect response, such as 301 (Moved Permanently), this
* indicates the minimum time that the user agent is asked to wait before
* issuing the redirected request.
*
* @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
*/
export function retryAfterDelay(response: { headers?: { "retry-after": string } }, lastRequestTime: number): number {
let retryAfterInMs = 0;
if (response) {
const retryAfter = response.headers["retry-after"];
if (/\d+/.test(retryAfter)) {
retryAfterInMs = ~~retryAfter * 1000;
}
if (
typeof retryAfter === "string" &&
isNaN(new Date(retryAfter as string).getTime()) === false
) {
retryAfterInMs = new Date(retryAfter).getTime() - lastRequestTime;
}
}
return retryAfterInMs;
}
and in our axiosRetry adaptation I use it like this:
if (shouldRetry) {
request.retryCount += 1;
const delay = retryAfterDelay(error.response, request.lastRequestTime) || retryDelay(request.retryCount, error);
So I dont wait additionally to the retryDelay but instead of the retryDelay. This makes more sense, as it would not add up the delays created by the retryDelay method.
@Softonic Will this feature be added?
I believe this is something you can workaround with the current API of the library Please feel free to create a new one if you think this is still relevant
Add error 429 to isRetryableError(). Add retryAfter() to handle Retry-After header. Handle negative delay from retryDelay() as cancellation. Add example for checking Retry-After against limit.