feat: retry timeout errors #34284
feat: retry timeout errors #34284
Conversation
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
|
I'd prefer Yury to take a look. |
Test results for "tests 1"1 failed 4 flaky37576 passed, 648 skipped Merge workflow run. |
| @@ -134,7 +134,7 @@ Defaults to `20`. Pass `0` to not follow redirects. | |||
| * since: v1.46 | |||
| - `maxRetries` <[int]> | |||
|
|
|||
| Maximum number of times network errors should be retried. Currently only `ECONNRESET` error is retried. Does not retry based on HTTP response codes. An error will be thrown if the limit is exceeded. Defaults to `0` - no retries. | |||
| Maximum number of times network errors should be retried. Only `ECONNRESET` and timeout errors are retried. Does not retry based on HTTP response codes. An error will be thrown if the limit is exceeded. Defaults to `0` - no retries. | |||
There was a problem hiding this comment.
What are the timeout errors that are retried? Does it now mean that overall timeout is maxRetries x timeout? I'm not sure I like the latter. If we want to support this, we could just add ETIMEDOUT (socket timeouts) to the retryable error codes and keep old semantics of the timeout parameter, i.e. if ETIMEDOUT occurs and the deadline is not reached yet (and retries count < maxRetries), we will retry, otherwise we bail out. I don't think we want to retry errors where the server received the request but was got stuck processing it and was not able to finish its response on time.
There was a problem hiding this comment.
that makes sense. fiddling with the deadline parameter already felt wrong to me. i'll rewrite it to only ETIMEDOUT.
There was a problem hiding this comment.
After looking into this more, I think that won't work. ETIMEDOUT is the error that occurs when a socket idles for a specified amount of time. By default, there's no timeout. So unless we add API for configuring socket timeout, we won't be able to retry ETIMEDOUT.
Most other forms of timeout i've tested show up as ECONNRESET, so they're already being retried today.
You're saying "we don't want to retry when the server got stuck processing", but given this I'm pretty sure that's what the user report is about. Let's discuss this more tonight.
Closes #34207