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

Add the retryOnTimeout option #47

Closed
wants to merge 2 commits into from

Conversation

ValentinH
Copy link

As requested in #39

@yns01
Copy link
Collaborator

yns01 commented May 6, 2018

@jlopezxs Did you have a chance to look at this PR? It's related to issue #39

@ValentinH ValentinH force-pushed the retry-on-timeout branch 2 times, most recently from 97d9018 to afad5d3 Compare May 30, 2018 14:31
@ValentinH
Copy link
Author

Hi @yns01 , I have refactored the timeouts handling a bit: the check for timeouts is only done in one place.

However, now the isNetworkOrIdempotentRequestError() function might not be named correctly and some functions behavior have changed.

@jlopezxs
Copy link
Contributor

jlopezxs commented Jun 4, 2018

This is nice but I think you can do that with retryCondition. I don't know if is good idea start creating Boolean parameters only to apply a "custom retry logic".

@ValentinH
Copy link
Author

I understand that we can override the retryCondition with custom logic. However, I ended up copying the isRetryableError, isIdempotentRequestError and isNetworkOrIdempotentRequestError in my code just for removing the checks about error.code !== 'ECONNABORTED'. This doesn't feel right.

I also understand that the solution I propose isn't the best one but I couldn't find another way so far... :/

@ValentinH
Copy link
Author

Any chance we could reconsider this PR?

@ValentinH
Copy link
Author

It's actually even more important than what I thought and I might want to rename the option to retryOnConnAborted: axios/axios#1543

Indeed, axios uses ECONNABORTED for timeouts but also for actual connections being aborted (for other reasons). This is why we might want to retry in this case.

@mathiasose
Copy link

mathiasose commented May 29, 2019

+1, I would also like to see this feature released.

In the meantime, this config should do the trick right?

axiosRetry(axios, {
  retries: 3,
  shouldResetTimeout: true,
  retryCondition: (error) => isNetworkOrIdempotentRequestError(error) || error.code === 'ECONNABORTED',
});

Technically this config would also retry non-idempotent requests (e.g. POST), but I am only going to use this config with a specific PUT operation, so it should be safe right?

@ruscon
Copy link

ruscon commented Jul 20, 2020

Have the same issue, can we fix it ?

@eddyson1006
Copy link

eddyson1006 commented Apr 9, 2021

+1, I would also like to see this feature released.

In the meantime, this config should do the trick right?

axiosRetry(axios, {
  retries: 3,
  shouldResetTimeout: true,
  retryCondition: (error) => isNetworkOrIdempotentRequestError(error) || error.code === 'ECONNABORTED',
});

Technically this config would also retry non-idempotent requests (e.g. POST), but I am only going to use this config with a specific PUT operation, so it should be safe right?

Yeah so far the only solution to retry on timeout.

@ValentinH ValentinH closed this Apr 19, 2021
@ValentinH ValentinH deleted the retry-on-timeout branch April 19, 2021 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants