sindresorhus / ky

🌳 Tiny & elegant JavaScript HTTP client based on the Fetch API
MIT License
13.62k stars 363 forks source link

Support retry delay with jitter? #509

Open rileytomasek opened 1 year ago

rileytomasek commented 1 year ago

Thanks for creating ky! I used it extensively in my libraries and projects.

I'm running into an issue where I'm making a lot of calls in parallel to an API with a rate limit and the 429 retries are all happening at the same time and triggering the rate limit again. I've tested with a forked version of Ky and adding a random jitter to the retry time makes things much better.

I'm happy to contribute a PR, but wanted to see which, if any, solutions you are open to:

  1. Add a random jitter (and potentially a way to control it) to the retry calculation here
  2. Allow passing a custom function to replace BACKOFF_FACTOR * (2 ** (this._retryCount - 1)) * 1000
  3. Make it possible to do this from the beforeRetry hook?

For more context, what I tested was calculating a jitter factor like JITTER_FACTOR * randomBetween(1, -1) and then multiplying the existing retry time by that. It could be configured so that the default value is 1, so there is no change to existing retry time calculation.

sindresorhus commented 1 year ago

Yes, I'm open to all of this. 👍

sindresorhus commented 11 months ago

@rileytomasek Only number 2 of these requests have been done.

rileytomasek commented 11 months ago

@sindresorhus can’t number 1 be accomplished by adding jitter to the delay function? I’m not familiar with the beforeRetry hook to know if you could do it there, but is it required if you have 1 and 2?

sindresorhus commented 11 months ago

It can yes, but I think it could still be useful to have it built-in.

sholladay commented 1 month ago

Any thoughts on how to do 3?

We could allow beforeRetry to return the ms to delay. But that means the hooks must run before the delay rather than after it, which is arguably a breaking change and IMO less intuitive.

sindresorhus commented 1 month ago

We could allow beforeRetry to return the ms to delay. But that means the hooks must run before the delay rather than after it, which is arguably a breaking change and IMO less intuitive.

It could potentially apply to the next retry instead. You don't really need a jitter for the first retry.

Alternatively, skip this and only do a jitter function. Something like:

retry: {
    delay: 1000,
    jitter: delay => delay * (0.8 + Math.random() * 0.4)
}