retry-handler should pass any error to retry method

[fatal10110]

if we pass AbortSignal with timeout, to limit the request time, in case of a timeout, the controller will be aborted in this case, retry handler assumes there is no reason to retry the request.

I want to move the decision to the user, the user may decide to mutate the "opts", pass new AbortSignal, and retry the request.

The implementation should look like...

Need to change

if (controller?.aborted || isDisturbed(this.opts.body)) {
  this.handler.onResponseError?.(controller, err)
  return
}

to

if (isDisturbed(this.opts.body)) {
  this.handler.onResponseError?.(controller, err)
  return
}

then use can do

new RetryAgent(new Agent(), {
  retry: (err, { state, opts }, cb) => {
    if (err.code === DOMException.TIMEOUT_ERR)
       opts.signal = AbortSignal.timeout(...)
       cb()
       return
    }
    
    cb(err)
  }
})

[metcoder95]

Would you like to send a PR for it? Do not forget to add tests

[artur-ma]

Looks like the solution is not compatible with api-request https://github.com/nodejs/undici/blob/4ab315e2e123f4e6ebcb7d859cb8530e7c76c622/lib/api/api-request.js#L66

once it go a signal, it is considered aborted since it has a "reason" set

[metcoder95]

Hmm, the RequestAborted can be extended with a reason that helps identify what cause it; or even create a new one.

Feel free to open a PR for that

[fatal10110]

Hmm, the RequestAborted can be extended with a reason that helps identify what cause it; or even create a new one.

Feel free to open a PR for that

Thats not the issue :)

it just aborts in onConnect if there is a reason

https://github.com/nodejs/undici/blob/4ab315e2e123f4e6ebcb7d859cb8530e7c76c622/lib/api/api-request.js#L77-L79

Changing that, would be dangerous, the only way to do that is to change api-request to check signal reference if aborted

[metcoder95]

Get what you mean, nevertheless I was referring to extend the RequestAbortError to enrich its context, without affecting the reason implementation.

For undici, a request being aborted means an actual intention of the caller to abort the request, and for the assumption of the handler is to not try anymore as the caller is not interested in the request any longer.

If aiming to rely on response timeouts (request sent, and waiting for a response), it is better to use the timeout settings while instantiating an Agent or a Client-based dispatcher.

[artur-ma]

Get what you mean, nevertheless I was referring to extend the RequestAbortError to enrich its context, without affecting the reason implementation.

For undici, a request being aborted means an actual intention of the caller to abort the request, and for the assumption of the handler is to not try anymore as the caller is not interested in the request any longer.

If aiming to rely on response timeouts (request sent, and waiting for a response), it is better to use the timeout settings while instantiating an Agent or a Client-based dispatcher.

well the agent timeouts as mentioned here by @mcollina is not for users, its more "security mechanism" and it doesn not support sub-seconds value

[metcoder95]

Indeed, tho for the use-case shared here, these timeouts (high level and not precise) fits better than an AbortSignal, given that the signal itself works better to control the flow of async executions, and aborting a request indeed means caller intention to abort a request and the assumption of the retry-handler seems correct.

[fatal10110]

@metcoder95 Then what is expected on timeout when I want to retry my request? I want to set timeout on my request, I can not do it via agent option, I have to do it via abort controller, but in this case I can not retry the equest

Not sure how it is expeced

I will give you an example, I have open connection to origin, but if this specific instance of the origin is overloaded, in this case I expect to get timeout error, then retry the request, and hopefully get another connection to another instance of the origin

The correct way to do it, is to set headersTimeout, them retry-handler will catch it. but as mentioned before, headersTimeout has limitations, and is not inteded to be used for this case.

Then the only option I have, is to set abort controller, and get the request aborted on timeout, but in this case retry-handler is useless, so i have to implement the retry logic by myself. Retries on timeouts for idenpotent methods, is pretty common

[metcoder95]

The RetryHandler or Agent, can fully do that based on the errors thrown by the headersTimeout or the requestTimeout; these will be identified as potential errors and will attempt to retry the request again accordingly to the settings.

Using AbortSignal, as said early, should be mostly scoped to intentionally abort a request already in flight; if the request goes timed out after connection or the server accepted the request but haven't serve the response yet, its better to rely on these connection timers instead (although under high concurrent traffic they can be a bit misleading).

[artur-ma]

these will be identified as potential errors and will attempt to retry the request again accordingly to the settings.

But headersTimeout just do not work as expcted, this is what im trying to tell, u can not set 100 ms headers timeout, just cant :)

https://github.com/nodejs/undici/blob/be11b7dfa9f45335d4ab1a81bb0b13cd4923143d/lib/util/timers.js#L24-L30

[metcoder95]

Sure, I'm not disregarding that; its a limitation. My recommendation is for common situations and overall recommendation. For more narrowed use-cases like this one, it might be better to create a custom implementation (as stated by this issue description)