ky icon indicating copy to clipboard operation
ky copied to clipboard

Using "Retry-After" with error code 425 is impossible

Open yaaax opened this issue 3 years ago • 6 comments

Hi there,

I would like to change the way the retries are made. For example: wait 1s before each retry.

One option is to add a retry.backoffLimit option Another (by the time the PR is merged) would be to use the Retry-After header from the server. But unfortunately I can make it work with the 425 error code given by the server. It looks like the afterStatusCodes option, set by default to [413, 429, 503] cannot be changed. I've tried these retry options :

retry: {
  limit: 12,
  afterStatusCodes: [425],
  statusCodes: [425],
}

The code I'm looking at is here: (Ky.ts) _calculateRetryDelay l.217

afterStatusCodes is not in the docs… but I found it reading the code and in that article

Any help is welcome

yaaax avatar Nov 28 '22 22:11 yaaax

Allowing to override afterStatusCodes would be really helpful.

Current implementation doesn't allow to override it: https://github.com/sindresorhus/ky/blob/53ce279c818998ed372822207b967eb68b92fba6/source/utils/normalize.ts#L39-L43

Removing line 42 will fix the issue.

mahnunchik avatar Mar 17 '23 12:03 mahnunchik

Curious what the use-case here was. 425 status codes are really a part of the initial handshake process and should be handled by either the browser/Node themselves through an immediate connection reset as specified in 8470 section 4 and demonstrated in the Firefox implementation. A 425 falling down into the JS seems like it would indicate a configuration issue or bug in the server/environment.

Of course, allowing users to set afterStatusCodes is still a good idea. Especially since it's broadcast as part of the public type interface.

Kenneth-Sills avatar Mar 27 '24 04:03 Kenneth-Sills

@sindresorhus ping

mahnunchik avatar Mar 28 '24 03:03 mahnunchik

@mahnunchik The maintainer responds more to PRs than to issues (they have... uh... a few repositories). In the next month or so I can probably get to this; unless you beat me to it, of course. 😄

Kenneth-Sills avatar Jun 24 '24 06:06 Kenneth-Sills

Is it possible for fetch() to return a 425? I agree with @Kenneth-Sills, this seems like it ought to be out of scope. But I’m okay with the above mentioned solution if it’s indeed possible for fetch() to handle it.

If so, you should be able to implement this already with a beforeRetry hook.

We await hooks before doing the retry. So just return a promise that resolves when you want us to retry. For example, wrapsetTimeout() in a promise and use error.response.headers to get the Retry-After value for the timeout.

sholladay avatar Jun 24 '24 20:06 sholladay

Ah, tracing back the origin of that offending line, it seems there was actually a discussion about this very thing back in 2019:

Q: Should this be exposed? A: No... We have not exposed it in Got either and no one has requested it yet.

But we did end up accidentally exposing it, and seems like the article OP linked has at least a couple of users thinking it's a part of the intended public interface. This also means IDE autocomplete for TS has been broadcasting this "customizable" property for the whole time.

In light of that, I still think it's best to just let users overwrite the property instead, rather than going through the trouble to artificially impose an Exclude on the options they're allowed to pass in. It's one line and some documentation, after all.


Relevant PR Created.

Kenneth-Sills avatar Jun 25 '24 02:06 Kenneth-Sills