Setting `retry.calculateDelay` option effectively clears other useful retry defaults

[davidtheclark]

Describe the bug

• Node.js version: v12.16.0
• OS & version: macOS 10.15.3

Setting retry.calculateDelay option effectively clears retry.statusCodes, retry.errorCodes, retry.methods. So if I set retry.calculateDelay, I need to reimplement the status code, error code, and method filters.

I'd love to adjust the backoff algorithm with the retry.calculateDelay option while accepting the defaults for those other options. I like those other defaults, but the default backoff function is too conservative for my use case, and isn't capped.

I'm filing this as a bug because with the current documentation I am surprised by this behavior. Maybe you'd consider it a bug fix to clarify the documentation — in which case this is a feature request.

Is this a new occurrence of #753?

Below I'll explain one specific example of what I'd expect, to illustrate the broader point above:

Actual behavior

If I set a custom retry.calculateDelay function but do not set retry.statusCodes, errors are still filtered according to the default statusCodes values before they reach my retry.calculateDelay function. I will not retry on 404s, for example, even if I don't add any logic to my function to filter out 404s.

Expected behavior

My retry.calculateDelay function is called even if the status code is 404. Essentially, I need to reimplement the default status code, error code, and method filters.

Code to reproduce

const client = got.extend({
  retry: {
    limit: 10,
    calculateDelay: ({ attemptCount }) => {
      // I don't want to reimplement the code & method filters here. 
      if (attemptCount === 1) return 1;
      return Math.min(Math.pow(2, attemptCount) * 50 * Math.random(), 1000);
    },
  },
});

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[szmarczak]

Please read the documentation. You can access the computed value.

[davidtheclark]

@szmarczak I did read the documentation, and it is definitely not clear on this point. What is the computedValue? Where does it come from? Why would I want to use it? I guess it relates to this use case, but I had no idea from what's written there now. I don't know how anybody would guess that it is the way to use a calculateDelay function and get the defaults from other retry options.

[szmarczak]

I did read the documentation, and it is definitely not clear on this point.

I think you can clearly see here that the calculateDelay function returns computedValue by default, and as you read the following docs you can figure out that it is the default retry mechanism. The computedValue name is self-explanatory. If you still don't get it, IOW it is a value in milliseconds returned by the default retry mechanism.

Where does it come from?

I don't think the source code is so complicated to understand, have a look.

Why would I want to use it?

#1143

I don't know how anybody would guess that it is the way to use a calculateDelay function and get the defaults from other retry options.

Read this comment again.

[szmarczak]

If you're still struggling with this, here's an example on how to implement your problem:

{
    calculateDelay: ({computedValue, attemptCount}) => {
        if (computedValue) {
            return attemptCount * 1000;
        }

        return 0;
    }
}

[szmarczak]

You have not convinced me that the documentation is as clear as it could and should be and that I should have known better.

There's no need to know better as all the necessary info is already included in the docs. You just need to think logically:

if I've got computedValue and attemptCount, then I can return my own delay value only when computedValue is different than 0, so the original mechanism won't be altered.

Since you're not satisfied with the docs, feel free to send a PR :) That's what they stand for right? To make improvements.

[adabir2]

I've run into a similar issue where overriding retry.calculateDelay causes the sibling parameter retry.limit to be ignored. As I suspected based on the above thread, calculateDelay does more than the name suggests and the default implementation is where the limit is enforced, which is why overriding it causes the problem:
https://github.com/sindresorhus/got/blob/master/source/core/calculate-retry-delay.ts#L8

I don't think this is documentation issue. Ideally overriding calculateDelay would only affect the delay and not sibling parameters under retry. The other, IMO less ideal way of approaching it is to change retry so that instead of:
Type: number | object
It's:
Type: number | object | function
where you can do all your retry logic in that function to avoid this confusion.

In any case, I'm only passing through quickly while evaluating a few different libraries to replace the now deprecated request. If I choose to go with got, I may come back later and at least do a documentation PR.

[Giotino]

@adabir2 while I understand why the name calculateDelay could be a little misleading, its behavior is well described in the documentation (I know you don't think it's a documentation problem, but I just want to point it out for reference).

The calculateDelay property is a function that receives an object with attemptCount, retryOptions, error and computedValue properties for current retry count, the retry options, error and default computed value. The function must return a delay in milliseconds (or a Promise resolving with it) (0 return value cancels retry).

I think changing the API at this point is not a good idea. The calculateDelay function does everything you need (or at least everything that has been found useful until now) to control the retry logic.

[adabir2]

Thanks for the reply Giotino. I agree with you that changing the API at this point doesn't seem like a good idea. You're also absolutely right about how useful the calculateDelay function and its rich input set is for determining all retry logic. The equivalent one in superagent was quite a challenge to work with! So far I'm pretty impressed with got. Excellent work!

The issue, as you somewhat pointed out, is that the name hides the fact that the retry delay is being overloaded and used to control all retry logic.

What I was trying to do:

• Constant delay of 500ms between retries
• Limit of 2 retries

I read the doc including the paragraph you quoted and I thought I just had to set the limit, and set calculateDelay to () => 500. The input and output to calculateDelay is clear, but not how it affects the sibling retry parameters. I had seen things like this elsewhere in the docs, for example for http2, there's a note saying:

Note: Overriding options.request will disable HTTP2 support.

A small suggestion would be to do something similar for overriding calculateDelay.

[szmarczak]

Yeah, it makes sense to note that the function is responsible for handling the limit mechanism.