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: support retry-after header #142

Merged

Conversation

blakeyp
Copy link
Contributor

@blakeyp blakeyp commented Feb 13, 2021

Adds functionality to check for Retry-After header in error response and parse the value if it's available (as per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After)

  • Handles number and Date header values
  • Delays by parsed value up to max number of retries
  • Rejects immediately if header value could not be parsed/is invalid
  • Rejects immediately if header value is greater than configured maxRetryAfter (default 5 mins)
  • Feature can be toggled off via checkRetryAfter config param

Copy link
Owner

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

This is really great - thank you! Mostly a few test nits that would be nice to clean up before landing.

src/index.ts Outdated
// Or HTTP date time string
const dateTime = Date.parse(header);
if (!Number.isNaN(dateTime)) {
return dateTime - Date.now() + 3000; // Add 3sec buffer
Copy link
Owner

Choose a reason for hiding this comment

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

Huh - why the 3 second buffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that now as yeah it's not necessary :)

];
interceptorId = rax.attach();
const res = await axios({url});
assert.strictEqual(res.data, 'toast');
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't actually testing if the delays work. I know it's annoying, but could I trouble you to set up a test with a sinon timer, and verify it's doing what we expect? I've had to write similar tests in the past if you're looking for an example:
https://github.com/JustinBeckwith/linkinator/blob/main/test/test.retry.ts#L83

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using the same pattern - also set a higher default retry delay on the rax config to make sure the Retry-After value is actually being used

test/index.ts Outdated
try {
const cfg: rax.RaxConfig = {url, raxConfig: {maxRetryAfter: 1000}};
await axios(cfg);
} catch (e) {
Copy link
Owner

Choose a reason for hiding this comment

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

I know I may be using this pattern elsewhere in the test, but it's less than ideal. Would you mind using an assert.rejects instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@@ -78,7 +82,8 @@ describe('retry-axios', () => {
assert.fail('Expected to throw');
});

it('should retry at least the configured number of times', async () => {
it('should retry at least the configured number of times', async function () {
this.timeout(10000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to change the timeouts to the old function style after getting a weird error on the ci tests - https://github.com/JustinBeckwith/retry-axios/pull/142/checks?sha=f6f6c8d1bbe8b2a6da5301ee457fd6fe0970e121

@simllll
Copy link

simllll commented May 27, 2021

Anything we can do to get this feature merged? :)

@akshayrohatgi-hotstar
Copy link

@JustinBeckwith @blakeyp Any updates on this PR?

@JustinBeckwith JustinBeckwith merged commit 5c6cace into JustinBeckwith:main Jun 30, 2021
@akshayrohatgi-hotstar
Copy link

@JustinBeckwith @blakeyp Thanks a lot :)

@github-actions
Copy link

🎉 This PR is included in version 2.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants