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

Exponential and Fibonacci backoffs easily overflow #12

Closed
magnusbaeck opened this issue Feb 10, 2022 · 9 comments · Fixed by #13
Closed

Exponential and Fibonacci backoffs easily overflow #12

magnusbaeck opened this issue Feb 10, 2022 · 9 comments · Fixed by #13

Comments

@magnusbaeck
Copy link

If you let exponential or Fibonacci backoffs run for a while you'll easily get into an overflow situation. See https://go.dev/play/p/FUNKkZU0Kyn for an example. The NewExponential documentation says

It's very efficient, but does not check for overflow, so ensure you bound the retry.

but AFAICT you can't prevent overflows with WithCappedDuration since by then the overflow has already happened. How about changing said backoff functions to detect overflows and consistently return the largest time.Duration when the ceiling has been reached? Or am I missing something?

@sethvargo
Copy link
Owner

If you can figure out a way to do that such that it's still safe for concurrent use and doesn't introduce races, I'd be very interested :)

@magnusbaeck
Copy link
Author

Oooh, you want concurrency too. Should be feasible with a lock, or are those off limits?

@sethvargo
Copy link
Owner

All the backoffs are safe for concurrent use and need to remain that way. They currently use atomic operations and are incredibly fast. When I benchmarked it, introducing a mutex added a fairly large performance hit.

@magnusbaeck
Copy link
Author

Percentage-wise I'm sure that's true, but you'd have to run a really tight retry loop for that to have any significance.

I'm not sure what other options there are. It's not entirely easy to make WithCappedDuration to detect overflows (except for the special case of negative durations), and then we're down to adjusting the documentation to be clearer. Right now it's easy to read it as "just use WithCappedDuration and you'll be fine".

@sethvargo
Copy link
Owner

I think WithCappedDuration, specifically, can check for negative times and then apply the max.

@magnusbaeck
Copy link
Author

Yes, but retry.NewExponential(1 * time.Second) starts returning zero after a while (see https://go.dev/play/p/U0GEa6PRi4U) so we'd need a special case for that too. Bases must always be strictly greater than zero so applying the max for <= 0 would perhaps work out?

@sethvargo
Copy link
Owner

The problem is that Next() will continue to run, so eventually you'll stop overflowing

@magnusbaeck
Copy link
Author

Thanks for fixing this!

@github-actions
Copy link

This issue has been automatically locked since there has not been any
recent activity after it was closed. Please open a new issue for
related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants