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.

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

feat(retry): allow respecting Retry-After for user-defined status codes #598

Merged
merged 8 commits into from
Jul 25, 2024
Merged

feat(retry): allow respecting Retry-After for user-defined status codes #598

merged 8 commits into from
Jul 25, 2024

Conversation

Kenneth-Sills
Copy link
Contributor

Implemented as recommended in related issue, with appropriate testing and documentation.

The included test is just a copy of the existing 413 one, but with a 500 and required config instead.

Closes #473.

The test is just a copy of the existing 413 one, but with a 500 and
required config instead.

Closes #473.
Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a negative test, which verifies that Retry-After is not respected by default for a 500 response?

test/retry.ts Outdated
});

const result = await ky(server.url, {retry: {afterStatusCodes: [500]}}).text();
console.log(result);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log() should be removed

test/retry.ts Outdated
const retryAfterOn413 = 2;
const lastTried500access = Date.now();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you're just being consistent with the other, existing variables here but it seems like these should be local variables within the tests where they are needed.

If we had compute-intensive or async setup hooks/macros that were awaited before the tests, it could make a big difference as to what the start time is determined to be.

Copy link
Contributor Author

@Kenneth-Sills Kenneth-Sills Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to be honest I had tunnel vision and barely paid attention beyond getting it working with the existing paradigm. Apologies!

const result = await ky(server.url, {retry: {afterStatusCodes: [500]}}).text();
console.log(result);
t.true(Number(result) >= retryAfterOn500 * 1000);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing an assertion for the requestCount.

Looks like some of the other retry tests also have this mistake.

readme.md Outdated

If `retry` is a number, it will be used as `limit` and other defaults will remain in place.

If the response provides an HTTP status contained in `afterStatusCodes`, Ky will wait until the date or timeout given in the [`Retry-After`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After) header has passed to retry the request. If the provided status code is not in the list, the [`Retry-After`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After) header will be ignored (retries sent immediately).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retries sent immediately

That really depends on the retry configuration, so I would drop this part.

@sindresorhus
Copy link
Owner

You also need to update

ky/source/types/options.ts

Lines 118 to 142 in e044626

/**
An object representing `limit`, `methods`, `statusCodes` and `maxRetryAfter` fields for maximum retry count, allowed methods, allowed status codes and maximum [`Retry-After`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After) time.
If `retry` is a number, it will be used as `limit` and other defaults will remain in place.
If `maxRetryAfter` is set to `undefined`, it will use `options.timeout`. If [`Retry-After`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After) header is greater than `maxRetryAfter`, it will cancel the request.
By default, delays between retries are calculated with the function `0.3 * (2 ** (attemptCount - 1)) * 1000`, where `attemptCount` is the attempt number (starts from 1), however this can be changed by passing a `delay` function.
Retries are not triggered following a timeout.
@example
```
import ky from 'ky';
const json = await ky('https://example.com', {
retry: {
limit: 10,
methods: ['get'],
statusCodes: [413]
}
}).json();
```
*/
retry?: RetryOptions | number;

@Kenneth-Sills
Copy link
Contributor Author

Thanks for the review! I haven't had a lot of free time this past week, but I hope to get to this soon. Feel free to bump me if it gets stale, I do check notifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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