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

Add retry.backoffLimit option #454

Merged
merged 14 commits into from
Dec 12, 2022
Merged

Add retry.backoffLimit option #454

merged 14 commits into from
Dec 12, 2022

Conversation

fvanwijk
Copy link
Contributor

I saw in the source code that the retry backoff function is hardcoded and exponential. However, I wanted to customize it.
Fortunately I also found the open issue addressing this limitation.

To start simple, I added a backoffLimit so that you can cap the delay that is exponentially growing. After 5 or so retries the delay becomes so large that the user will give up anyway.
Setting the limit to for example 1000 in combination with 60 retries will change the behavior into polling for 1 minute (my use case).

I had some trouble testing the delay because the calculate function is private and I didn't find a way to extract this information from the ky instance or response.
So I did some perf measurements to test the delays.

If you are happy with the PR so far, I will implement the calculateDelay option too.

Fixes #389

source/types/retry.ts Outdated Show resolved Hide resolved
source/types/retry.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

CI is failing

@fvanwijk
Copy link
Contributor Author

Thanks for the review.

I must say that testing the delay of the retries a bit hard. You probably also know this, because the delay wasn't initially tested anyway.
Currently the test depends on a quick execution time, but if you run the test on a slow machine it might fail. In my opinion no test is better than a flaky test, so we should delete it.
Or are there ways to test the delay that I don't know of?

@sindresorhus
Copy link
Owner

Maybe check process.env.CI and increase the number a bit then?

@sindresorhus
Copy link
Owner

You also need to add the docs to the readme.

Frank van Wijk and others added 9 commits October 12, 2022 14:47
The default backoff function is exponential, so after a couple of retries the delay will become very large.
In case you are polling an api, you want at least that there is an upper limit to this delay, for example 1000ms.
readme.md Outdated Show resolved Hide resolved
source/types/retry.ts Show resolved Hide resolved
Copy link
Contributor Author

@fvanwijk fvanwijk left a comment

Choose a reason for hiding this comment

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

I updated the tests to use the Node 14 Performance API (instead of LTS).

By the way, it would be nice if CI runs automatically on push. Now the feedback cycle is pretty slow.

source/types/retry.ts Show resolved Hide resolved
@sindresorhus
Copy link
Owner

By the way, it would be nice if CI runs automatically on push.

I agree, but GitHub forces me to push this button each time.

Screenshot 2022-10-20 at 17 31 22

@sholladay
Copy link
Collaborator

GitHub forces me to push this button each time

That can be changed in the repository settings under Actions > General

image

@sindresorhus
Copy link
Owner

@sholladay I'm aware, and I did for this repo. But I have 1000+ repos. I cannot do it manually on all of them.

source/types/retry.ts Show resolved Hide resolved
source/types/retry.ts Outdated Show resolved Hide resolved
test/retry.ts Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
source/types/retry.ts Outdated Show resolved Hide resolved
@fvanwijk
Copy link
Contributor Author

fvanwijk commented Oct 31, 2022

It looks like the new test that depends on timing makes the test suite flaky. I doubt whether it is a good idea to keep the test. Removing it altogether might be the right thing to do.

@fvanwijk
Copy link
Contributor Author

@sindresorhus In the mean time there haven't been any activity for about a month. Any suggestions how to follow up? Changes needed or complete the PR?

@sindresorhus sindresorhus merged commit ab19baf into sindresorhus:main Dec 12, 2022
@sindresorhus
Copy link
Owner

Thanks. Sorry for the delay. I must have missed the notification.

@fvanwijk
Copy link
Contributor Author

This PR had flaky tests, so I increased the test timeout offsets. Maybe it is fixed in #484, so we could decrease (set to 0?) the allowedOffset.

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.

Customize retry backoff algorithm
3 participants