Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
yaaax opened this issue Nov 28, 2022 · 6 comments · May be fixed by #598
Open

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

yaaax opened this issue Nov 28, 2022 · 6 comments · May be fixed by #598

Comments

@yaaax
Copy link

yaaax commented Nov 28, 2022

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 yaaax changed the title Using "Retry-After" with error code 413 is impossible Using "Retry-After" with error code 425 is impossible Nov 28, 2022
@mahnunchik
Copy link

Allowing to override afterStatusCodes would be really helpful.

Current implementation doesn't allow to override it:

ky/source/utils/normalize.ts

Lines 39 to 43 in 53ce279

return {
...defaultRetryOptions,
...retry,
afterStatusCodes: retryAfterStatusCodes,
};

Removing line 42 will fix the issue.

@Kenneth-Sills
Copy link

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.

@mahnunchik
Copy link

@sindresorhus ping

@Kenneth-Sills
Copy link

@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. 😄

@sholladay
Copy link
Collaborator

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.

Kenneth-Sills added a commit to Kenneth-Sills/ky that referenced this issue Jun 25, 2024
@Kenneth-Sills
feat(retry): allow respecting Retry-After for user-defined status codes
f46f6cc 
The test is just a copy of the existing 413 one, but with a 500 and
required config instead.

Closes sindresorhus#473.
@Kenneth-Sills Kenneth-Sills linked a pull request Jun 25, 2024 that will close this issue
Open
@Kenneth-Sills
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(retry): allow respecting Retry-After for user-defined status codes Kenneth-Sills/ky
4 participants
@mahnunchik @sholladay @yaaax @Kenneth-Sills