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

Unexpected behavior in ExponentialStrategy #18

Closed
yamadashy opened this issue Aug 19, 2024 · 3 comments · Fixed by #19
Closed

Unexpected behavior in ExponentialStrategy #18

yamadashy opened this issue Aug 19, 2024 · 3 comments · Fixed by #19

Comments

@yamadashy
Copy link
Contributor

yamadashy commented Aug 19, 2024

The current implementation of ExponentialStrategy in the Backoff library produces an unexpected sequence of wait times. Instead of the conventional exponential backoff sequence (1, 2, 4, 8, ...), it generates a sequence of (1, 4, 8, 16, ...).

Current Behavior

Given a base wait time of 1 second (1000 ms), the current implementation results in:

  1. First attempt: 1 second
  2. Second attempt: 4 seconds
  3. Third attempt: 8 seconds
  4. Fourth attempt: 16 seconds

Expected Behavior

The expected exponential backoff sequence should be:

  1. First attempt: 1 second
  2. Second attempt: 2 seconds
  3. Third attempt: 4 seconds
  4. Fourth attempt: 8 seconds

Code Analysis

The issue stems from the current implementation in ExponentialStrategy:

public function getWaitTime($attempt)
{
    return (int) ($attempt == 1
        ? $this->base
        : pow(2, $attempt) * $this->base
    );
}

This implementation causes the exponential growth to start from the second attempt, resulting in the unexpected sequence.

Proposed Fix

To achieve the expected behavior, the getWaitTime method could be modified as follows:

public function getWaitTime($attempt)
{
    return (int) pow(2, $attempt - 1) * $this->base;
}

This change would produce the conventional exponential backoff sequence.

@yamadashy
Copy link
Contributor Author

If this behavior is unintended, I can prepare a pull request with a fix promptly. However, it's worth noting that such a change would be a breaking change.

@jszobody
Copy link
Member

Thank you for this! Definitely looks like a bug. I wrote that ExponentialStrategy so long ago, I don't recall why I did it that way.

Yes I would welcome a PR. And I'm not sure I would consider it breaking, it seems like a bugfix to me, making this behave the way it should have all along. And there is no impact to users of this library in terms of how they implement it, no API changes.

@yamadashy
Copy link
Contributor Author

yamadashy commented Aug 21, 2024

@jszobody
Hi,

I've put together a PR for this. Would really appreciate if you could take a look when you have a moment:

Thanks in advance!

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 a pull request may close this issue.

2 participants