Easier granular control of retry interceptor without losing built-in `retry-after` handling

[SukkaW]

This Would Solve...

I want more granular and flexible control over retry behavior. For example, instead of retrying only with specific error codes, I want to bail out of retries with specific error codes. Similarly, instead of retrying with specific http status codes, I want to bail out with specific http status code ranges (e.g., statusCode > 400 && statusCode < 599 && statusCode !== 401 && statusCode !== 403 && statusCode !== 404 && statusCode !== 405), or bail out of retries with a specific header.

Currently, I can accomplish this with the retry callback in the retry option, but if I provide a retry callback, I will lose undici's built-in retry-after header handling (which is in RetryHandler[kRetryHandlerDefaultRetry]):

https://github.com/nodejs/undici/blob/1bc83eaee7d743f8c3b52df880f5596c335b9818/lib/handler/retry-handler.js#L41 https://github.com/nodejs/undici/blob/1bc83eaee7d743f8c3b52df880f5596c335b9818/lib/handler/retry-handler.js#L107

I want to have both custom retry bailouts and retain undici's built-in retry-after handling.

The Implementation Should Look Like...

A shouldRetry callback in the retry option should receive context (which contains state and opt), header, method, statusCode, and errorCode. The callback should return a boolean or a number to determine whether the retry should continue and how long it should wait before the next retry.

I Have Also Considered...

Exposing RetryHandler[kRetryHandlerDefaultRetry] as RetryHandler['onRetryAfter'], which can be then called inside the custom retry callback.

[metcoder95]

The suggestion of the callback is exactly what we wanted to avoid to give more ergonomics to provide inversion of control and hint the handler when to execute the further retry.

For the handling of the retry-header, I believe we can abstract and export that logic into a function that can be used to handle such logic as utility, but I'd recommend avoid changing the way the callback works.

[mcollina]

@metcoder95 what's the problem with the callback approach?

[metcoder95]

Of returning boolean or number?

• That will mean another breaking change and deprecation notice for v6
• we wanted to enable use cases where caller wants to use async to calculate the retry timeout, to avoid spreading and trying to guess whether or not returns a Promise, we preferred to give the caller a callback that will trigger the next iteration; so they can wait for a Promise, or trigger a custom setTimeout and just exec the callback for the next try
  • Removing so will cause to us go back to verify what the function is trying to do
  • It kind of adds more complexity to the retry logic and callers loses more control over it

[SukkaW]

Of returning boolean or number?

• That will mean another breaking change and deprecation notice for v6

• we wanted to enable use cases where caller wants to use async to calculate the retry timeout, to avoid spreading and trying to guess whether or not returns a Promise, we preferred to give the caller a callback that will trigger the next iteration; so they can wait for a Promise, or trigger a custom setTimeout and just exec the callback for the next try

  • Removing so will cause to us go back to verify what the function is trying to do
  • It kind of adds more complexity to the retry logic and callers loses more control over it

@metcoder95 @mcollina

My proposal is not touching the existing retry callback for custom behavior.

An extra shouldRetry guard function that runs before the retry callback should be introduced. And we will move our existing statusCode and errorCodes logic in this new shouldRetry as default guard behavior.

This separates whether we should do a retry and actually do a retry into 2 functions.

[SukkaW]

But yes, simply exposing the built-in retry-after handling function is the easiest approach. People will provide their version of retry function, and in that retry function they can call retry-after handling function:

async retry (err, { state, opts }, cb) {
  // bail out error early with cb(err)
  // ...
  return cb(err);

  return RetryHandler.RetryAfter(err, { state, opts }, cb);
}

[metcoder95]

But yes, simply exposing the built-in retry-after handling function is the easiest approach. People will provide their version of retry function, and in that retry function they can call retry-after handling function:

async retry (err, { state, opts }, cb) {
  // bail out error early with cb(err)
  // ...
  return cb(err);

  return RetryHandler.RetryAfter(err, { state, opts }, cb);
}

I'm in sync with providing a RetryAfter utility. I'd prefer it to be just returning the resolution of parsing the retry-after header, but it could be interesting to see some more examples to see if a callback version works.

The shouldRetry coexisting with retry seems a bit odd API, as it can lead to ambiguities given that both seems to overlap at some extent.

[SukkaW]

I'd prefer it to be just returning the resolution of parsing the retry-after header

IMHO, if we're going to do that, we should return the retry timeout directly as undici also implements exponential backoff in the default retry callback.

Otherwise, when a custom retry callback is provided, the developer must manually implement the exponential backoff again.

[metcoder95]

That was somehow the point, provide out-of-box implementation that its a good default, and leave more customized implementations up to the implementer.

One thing that I've been thinking about, is to maybe provider other retry strategies out-of-the-box; e.g. linea retry, circuit breaker maybe, and more down the line.

The retry-after its a good idea to expose it, that doesn't change.