when providing retryOn function, checking attempts before retrying

[abrathovde]

No description provided.

[abrathovde]

@jonbern bump?

[jonbern]

Thanks for your pull request. What's the background for this change?

I see from the branch name it refers to infinite-loop-server-error. Do you get an infinite loop if you return false in your retryOn() function?

[abrathovde]

If the return from retryOn function is null/undefined/false the infinite loop does not occur. If the return is true then the infinite loop occurs. For example... the server call keeps failing

[jonbern]

That's the expected behaviour actually.

Also, the way I see it, is that defining retryOn() as a function is really an edge case, and normally it should be sufficient to not define anything, or define one or a small set of HTTP codes to retry on. In my opinion, that should cover most of the cases, but if it doesn't, the library gives you the option to define a retryOn() function, where you can implement your custom logic, and where the first argument is the attempt number.

[abrathovde]

Hm... I understand. Maybe it's a missed touch point as far as documentation goes. My understanding prior to getting into the code was that upon defining the number of retries and a custom retryOn function would not result in an infinite loop.

So you're saying that if the retryOn function is specified, the retries property has no value? The two should not be used together?

[jonbern]

Indeed, that is correct.

Personally, I've never needed to define retryOn as a function (note, it's a new addition to the lib). The only use case I can think of is if the API returns a generic error code, for instance 500, and you need to differentiate whether or not to retry based on a textual error message. But even that, I think it's not that useful, because if it's returning a generic error code, it's likely that it's going to occur every time you make the request (and if not, it should be improved in the API if possible).

Anyway, thanks for your PR and feedback regarding the documentation. I will see if it's possible to improve the documentation to make it more clear that retryOn() is an edge case.