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

retries-property not working when using axios v0.19.0-beta.1 #59

Closed
actraiser opened this issue Aug 25, 2018 · 24 comments
Closed

retries-property not working when using axios v0.19.0-beta.1 #59

actraiser opened this issue Aug 25, 2018 · 24 comments
Labels

Comments

@actraiser
Copy link

actraiser commented Aug 25, 2018

I noticed that the library will not honor the number in the retries-property when using the 0.19.0 beta release of Axios. The retryCount seems to always remain at 1 on errors, so axios-retry infinitely retries requests and never fails. Not sure what exactly causes the new behavior but here is some information on what changed in 0.19.0:

Axios Releases Page

I use axios-retry like so (typescript):

axiosRetry(axios, {
 retries: 5,
  retryCondition: (error: any) => {
    return axiosRetry.isNetworkOrIdempotentRequestError(error)
      || error.code === 'ECONNABORTED' || error.code === 'ENOTFOUND';
  },
  retryDelay: (retryCount: number) => {
    return retryCount * 1000;
  },
});

Greets
-act

@Fitzpasd
Copy link

The is being caused by the new mergeConfig implementation they are using. The retry code relies on injecting a axios-retry key into the config for use later. This is no longer valid since they aren't merging in any unknown keys.

@mawrkus
Copy link
Contributor

mawrkus commented Apr 19, 2019

If anyone figures out a solution and is willing to submit a PR to anticipate the 0.19.0 Axios release, it would be greatly appreciated!

@Joelkang
Copy link

@mawrkus I have a PR up for this: #76

Any thoughts about the implementation?

@jmicco
Copy link

jmicco commented Jun 4, 2019

Hi all! We stumbled across this when we updated axios to 0.19, it completely broke our retries. Will a new version be produced? the PR has been open for 25 days!

@jmicco
Copy link

jmicco commented Jun 4, 2019

NOTE: 0.19 axios went public 5 days ago!

@jkelin
Copy link

jkelin commented Jun 5, 2019

Despite bad manners, I would also like to weight in on this, as appears this to be the reason why axios-retry does not work for me with axios 0.19. Unfortunately axios-retry does not work for me with 0.18 either because axios config merge bug (which has been fixed in 0.19).

@jmicco
Copy link

jmicco commented Jun 12, 2019

Hi All!

If no one is going to push a PR to fix this library to work with Axios 0.19, then it is effectively dead and should be removed from NPM.

The maintainers should do something to make it compatible or I will advocate that it be deprecated on NPM.

Thanks!
John

@herrherrmann
Copy link

herrherrmann commented Jun 12, 2019

Hi @jmicco,

with the risk of starting a very digressing debate: Your comment sounds very threatening and disrespectful. If you intended this interpretation, I believe you're ungrateful and don't understand how open source software development works. If you didn't intend this interpretation, please think about a more diplomatic way to "push" (rather: motivate) the release of this fix.

As you saw, there's a PR in the works, so it's not like anyone is actively blocking or delaying this fix. If you can, you should contribute to it, e.g. by suggesting changes or reviewing the PR.

I'm also relying on axios and this plugin, for now I downgraded axios to 0.18.1 to address this issue temporarily. Of course it's not ideal and one of the hassles with npm-based libraries. But I'm also happy I can stand on the shoulders of giants this way. :)

@jmicco
Copy link

jmicco commented Jun 12, 2019

Sorry, I agree that my tone was not great. and I really do want to keep the goodness of open source available for everyone and to be able to continue to leverage this package.

I do not know enough about the internals to either review the change or contribute a better fix, but this package has not accepted any PR in about 5 months, the PR with the fix has been open for more than 1 month and the bug has been open for 10 months. The bug renders the package useless in a completely non-obvious way - which could cause huge problems - retrying every 200 ms forever in the default configuration. It would be better and more obvious if it simply failed to compile.

Its hard for me to believe that this project is viable if it no longer works with the latest public version of axios, and a PR has not been accepted to fix that in 10 months since the problem was first identified.

@mawrkus
Copy link
Contributor

mawrkus commented Jun 12, 2019

Hello everyone,

First of all, apologies for the lack of feedback/support on this particular issue and, in general, on the whole project.

To give you a bit of context, we have been struggling these last months to do a proper job on our OSS projects. Defining ownership, organising the work to be done and finding the time to do it properly is not a trivial process... Indeed, we are no different than many OSS contributors: we have a fulltime 40h/week job, deadlines to meet and a personal life to take care of (and to enjoy as much as possible). Answering comments, fixing issues, implementing new features and finding ways to build great OSS software happen between those lines. I'm sure you understand this.

Now, the good news is that we are going to be able to handle these issues faster from now on and we hope that it will benefit the OSS community as much as we have benefited from it so far.

Saying that, it looks like #76 is still in WIP. We'll check with @jlopezxs how we can help to merge it asap.

Thank you for your patience and support!

@Joelkang
Copy link

Joelkang commented Jun 13, 2019

@mawrkus and @jlopezxs I've updated the implementation which passes tests. My biggest concern right now is wrapping the adapter function so that different requests don't actually share the same state object: https://github.com/softonic/axios-retry/pull/76/files#r293362198

@jlopezxs
Copy link
Contributor

To solve this issue prefer to add the configuration in the client itself, loosing the axios-retry config as a second parameter #76 (comment).
#82

@usb248
Copy link

usb248 commented Jun 21, 2019

Any fix for infinite loop axios retry ?

@alexkrolick
Copy link

FYI Axios 0.19 got pushed to stable due to a security patch a few weeks ago, so a lot of clients are now upgraded and experiencing infinite loops

@thatmarvin
Copy link

Fyi there's a PR on axios that relaxes mergeConfig to reenable custom config: axios/axios#2207. Sounds like axios-retry doesn't need to do anything if that PR goes through.

@alexkrolick
Copy link

Proposal: can the defaults be changed to "fail closed" (not retry at all or retry with a low maximum count) instead of infinitely retrying?

@akirilyuk
Copy link

Are there any updates on this? Will the lib be updated to support axios 19.0 or will you wait for axios to fix this on their side?

@peterwang-s
Copy link

I simply skipped over the problem, hoping it would be helpful

let retries = 0
axiosRetry(axios, {
    retries: 3,
    retryCondition:(error)=>{
      let config = error.config;
      if (!config) {
        return false;
      }
      retries = retries + 1
      if(retries>= 3){
        retries = 0
        return false
      }
      return true
      
      // do something 
      // axiosRetry.isNetworkError
      // axiosRetry.isRetryableError
      //axiosRetry.isSafeRequestError
      // axiosRetry.isIdempotentRequestError
      // axiosRetry.isNetworkOrIdempotentRequestError
      // axiosRetry.exponentialDelay
    }
  });

@akirilyuk
Copy link

@redredredredredred What will happen if you have 2 simultaneous requests failing? They share the same retries number so they will fail after 1 and 2 retries?

@victordidenko
Copy link

I'm implementing my own mechanism to retry requests, and stumbled upon this issue (Axios ignores non-standard keys in config).
I chose dead-simple way and decided to store tries in request header Tries. This is non-standard header, so it shouldn't break anything, and Axios doesn't (and will never) remove headers from config :)

@chase-si
Copy link

chase-si commented Nov 6, 2019

I have to set axios back to 0.18.
It works

@usb248
Copy link

usb248 commented Nov 6, 2019

@chase-si the same... I can't update axios owing to this bug

@matushorvath
Copy link

It seems this will be fixed in axios 19.1 once it is released, see:
axios/axios#2438
axios/axios#2502

@hsxfjames
Copy link

hsxfjames commented Jan 9, 2020

axios@0.19.1 has been released axios/axios#2502 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet