feat(retry): Add configurable delay

[benlesh]

• Adds a delay configuration that allows the user to create simpler exponential backoff, simple retry delays, and/or other functionality.

Basically, this will allow a user to write an exponential backoff or simple delay more easily and intuitively than retryWhen could. In fact, we might want to deprecate retryWhen in favor of this, and also add this pattern to repeat (if we decide it's a good idea).

Retry every 5 seconds, a maximum of 100 times:

source.pipe(
  retry({
     // max retries (count is an unfortunate existing name)
     count: 100, 
     // retry 5 seconds after any error
     delay: 5000, // retry after 5 seconds
  })
)

Exponential backoff example:

source.pipe(
  retry({
    // max 100 retries
    count: 100,
    // backoff starting at 2 seconds, exponentially up to 1 minute.
    // retryCount starts a 1. 
    delay: (_err, retryCount) => timer(Math.min(60000, 1000 * (2 ** retryCount)))
  })
)

Notes

Honestly, I'm okay even with adding configuration for the above backoff scenario, given it's such a common thing I've had to implement. Even if all we did was provide a helper function that was easy to tree shake like: retry({ delay: exponentialBackoff({ startDelay: 1000, maxDelay: 60000 }) }) or the like.

[jakovljevic-mladen]

Seems like this sentence is not finished.

[benlesh]

This is so weird. I feel like this is the third or fourth time this has happened to me, but only with doc comments :\

[backbone87]

isnt retryWhen the more general operator which can be improved by providing premade notifier factories?

// like `retry`
retryWhen(count(n));
retryWhen(count({ count: n, resetOnSuccess: true }));

// exponential backoff
retryWhen(exponentialBackoff({ startDelay: d, maxDelay: m }));

// custom, like current `retryWhen`
retryWhen(createMyCustomNotifier);

[cartant]

isnt retryWhen the more general operator which can be improved by providing premade notifier factories?

I somewhat agree with this, but AFAICT the following would not be possible with retryWhen as it has no mechanism for conveying 'success' to the notifier:

retryWhen(count({ count: n, resetOnSuccess: true }));

I'm more curious as to whether this PR hints at a general approach for reducing the number of core operators by making them more 'configurable' - as was done with share and the multicasting API.

[backbone87]

general approach for reducing the number of core operators

i think the current state of share & shareReplay are a good example. a "highly configurable" base operator with the most common configuration as its default and if necessary additional operators for other very common configurations that are implemented in terms of the "base" operator.
imho the benefits of this approach is less code duplication, less tests required and most importantly no "slightly different behavior under equal configuration" for otherwise very similar operators.
from the idea to provide "option factories" (exponentialBackoff): should it then also be share(replay(n))?

the following would not be possible with retryWhen as it has no mechanism for conveying 'success' to the notifier

funnily enough that hinted me into finding a bug in the custom retry backoff operator that I use and caused various services to restart without "retrying enough" - because the attempt counter was not reset. but I noticed immediately the follow up question: what is success? or when should the attempt counter actually be reset?

[benlesh]

Notes from core team: General approval.

[cartant]

I think this is fine, but I'm not sure about the resetOnSuccess name:

  /**
   * Whether or not to reset the retry counter on success.
   * Defaults to false.
   */
  resetOnSuccess?: boolean;

What is success? The accompanying doc/comment doesn't make this clear. The other reset options in the API are, IMO, a little clearer: resetOnError, resetOnComplete, resetOnDisconnect, resetOnRefCountZero.

I mean, from resetOnSuccess a caller could infer that it could only mean that it'll reset when a next notification is received - because resetting on complete would be meaningless - but should the caller really need to even think about this?

Maybe resetOnNext?

[benlesh]

@cartant I've changed the one setting to resetOnFirstValue. Since resetOnSuccess doesn't mean much, and resetOnNext is ambiguous.

[benlesh]

@cartant Just realized that resetOnSuccess was pre-existing. Probably should change that back.