-
Notifications
You must be signed in to change notification settings - Fork 2
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
Introduce Backoff
implementation with exponential strategy
#50
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,176 @@ | ||
<?php | ||
|
||
namespace ipl\Stdlib; | ||
|
||
use Exception; | ||
use LogicException; | ||
|
||
class ExponentialBackoff | ||
{ | ||
/** @var int The minimum wait time for each retry in ms */ | ||
protected $min; | ||
|
||
/** @var int The maximum wait time for each retry in ms */ | ||
protected $max; | ||
|
||
/** @var int Number of retries to be performed before giving up */ | ||
protected $retries; | ||
|
||
/** @var ?int The previous used retry wait time */ | ||
protected $previousWaitTime; | ||
|
||
/** | ||
* Create a backoff duration with exponential strategy implementation. | ||
* | ||
* @param int $retries The number of retries to be used before given up. | ||
* @param int $min The minimum wait time to be used in milliseconds. | ||
* @param int $max The maximum wait time to be used in milliseconds. | ||
yhabteab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
public function __construct(int $retries = 1, int $min = 0, int $max = 0) | ||
{ | ||
$this->retries = $retries; | ||
|
||
$this->setMin($min); | ||
$this->setMax($max); | ||
} | ||
|
||
/** | ||
* Get the minimum wait time | ||
* | ||
* @return int | ||
*/ | ||
public function getMin(): int | ||
{ | ||
return $this->min; | ||
} | ||
|
||
/** | ||
* Set the minimum wait time | ||
* | ||
* @param int $min | ||
* | ||
* @return $this | ||
*/ | ||
public function setMin(int $min): self | ||
{ | ||
if ($min <= 0) { | ||
$min = 100; // Default minimum wait time 100 ms | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please introduce consts for the defaults. |
||
} | ||
|
||
$this->min = $min; | ||
|
||
return $this; | ||
} | ||
|
||
/** | ||
* Get the maximum wait time | ||
* | ||
* @return int | ||
*/ | ||
public function getMax(): int | ||
{ | ||
return $this->max; | ||
} | ||
|
||
/** | ||
* Set the maximum wait time | ||
* | ||
* @param int $max | ||
* | ||
* @return $this | ||
* @throws LogicException When the configured minimum wait time is greater than the maximum wait time | ||
*/ | ||
public function setMax(int $max): self | ||
{ | ||
if ($max <= 0) { | ||
$max = 10000; // Default max wait time 10 seconds | ||
} | ||
|
||
$this->max = $max; | ||
|
||
if ($this->min > $this->max) { | ||
throw new LogicException('Max must be larger than min'); | ||
} | ||
|
||
return $this; | ||
} | ||
|
||
/** | ||
* Get the configured number of retries | ||
* | ||
* @return int | ||
*/ | ||
public function getRetries(): int | ||
{ | ||
return $this->retries; | ||
} | ||
|
||
/** | ||
* Set number of retries to be used | ||
* | ||
* @param int $retries | ||
* | ||
* @return $this | ||
*/ | ||
public function setRetries(int $retries): self | ||
{ | ||
$this->retries = $retries; | ||
|
||
return $this; | ||
} | ||
|
||
/** | ||
* Get a new wait time for the given attempt | ||
* | ||
* If the given attempt is the initial one, the min wait time is used. For all subsequent requests, | ||
* the previous wait time is simply multiplied by 2. | ||
* | ||
* @param int $attempt | ||
* | ||
* @return int | ||
*/ | ||
public function getWaitTime(int $attempt): int | ||
{ | ||
if ($attempt === 0) { | ||
$this->previousWaitTime = null; | ||
} | ||
|
||
if ($this->previousWaitTime >= $this->max) { | ||
return $this->max; | ||
} | ||
|
||
$next = min(! $this->previousWaitTime ? $this->min : $this->previousWaitTime * 2, $this->max); | ||
$this->previousWaitTime = $next; | ||
|
||
return $next; | ||
} | ||
|
||
/** | ||
* Execute and retry the given callback | ||
* | ||
* @param callable(?Exception $err): mixed $callback The callback to be retried | ||
* | ||
* @return mixed | ||
* @throws Exception When the given callback rethrows an exception that can't be retried or max retries is reached | ||
*/ | ||
public function retry(callable $callback) | ||
{ | ||
$attempt = 0; | ||
$previousErr = null; | ||
|
||
do { | ||
try { | ||
return $callback($previousErr); | ||
} catch (Exception $err) { | ||
if ($attempt >= $this->getRetries() || $err === $previousErr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the logic for deciding whether something is retryable not part of this code, but must be executed in each individual callback? With the current implementation, the decision whether something is retryable is made after sleeping (the next time the callback is called), and the first attempt cannot stop the retryable logic (please add a test). And requires duplicate code in each individual callback. |
||
throw $err; | ||
} | ||
|
||
$previousErr = $err; | ||
|
||
$sleep = $this->getWaitTime($attempt++); | ||
usleep($sleep * 1000); | ||
} | ||
} while ($attempt <= $this->getRetries()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
<?php | ||
|
||
namespace ipl\Tests\Stdlib; | ||
|
||
use Exception; | ||
use ipl\Stdlib\ExponentialBackoff; | ||
use LogicException; | ||
|
||
class ExponentialBackoffTest extends \PHPUnit\Framework\TestCase | ||
{ | ||
public function testInvalidMaxWaitTime() | ||
{ | ||
$this->expectException(LogicException::class); | ||
$this->expectExceptionMessage('Max must be larger than min'); | ||
|
||
new ExponentialBackoff(1, 500, 100); | ||
} | ||
|
||
public function testMinAndMaxWaitTime() | ||
{ | ||
$backoff = new ExponentialBackoff(); | ||
$this->assertSame(100, $backoff->getMin()); | ||
$this->assertSame(10 * 1000, $backoff->getMax()); | ||
|
||
$backoff | ||
->setMin(200) | ||
->setMax(500); | ||
|
||
$this->assertSame(200, $backoff->getMin()); | ||
$this->assertSame(500, $backoff->getMax()); | ||
} | ||
|
||
public function testRetriesSetCorrectly() | ||
{ | ||
$backoff = new ExponentialBackoff(); | ||
|
||
$this->assertSame(1, $backoff->getRetries()); | ||
$this->assertSame(5, $backoff->setRetries(5)->getRetries()); | ||
$this->assertNotSame(10, $backoff->setRetries(5)->getRetries()); | ||
} | ||
|
||
public function testGetWaitTime() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should also test the waiting time for attempt |
||
{ | ||
$backoff = new ExponentialBackoff(100, 1000); | ||
|
||
$this->assertSame($backoff->getMin(), $backoff->getWaitTime(0)); | ||
$this->assertGreaterThan($backoff->getWaitTime(0), $backoff->getWaitTime(1)); | ||
$this->assertGreaterThan($backoff->getWaitTime(1), $backoff->getWaitTime(2)); | ||
$this->assertSame($backoff->getMax(), $backoff->getWaitTime(3)); | ||
} | ||
|
||
public function testExecutionRetries() | ||
{ | ||
$backoff = new ExponentialBackoff(10); | ||
$attempt = 0; | ||
$result = $backoff->retry(function (Exception $err = null) use (&$attempt) { | ||
if (++$attempt < 5) { | ||
throw new Exception('SQLSTATE[HY000] [2002] No such file or directory'); | ||
} | ||
|
||
return 'succeeded'; | ||
}); | ||
|
||
$this->assertSame(5, $attempt); | ||
$this->assertSame('succeeded', $result); | ||
} | ||
|
||
public function testExecutionRetriesGivesUpAfterMaxRetries() | ||
{ | ||
$this->expectException(Exception::class); | ||
$this->expectExceptionMessage('SQLSTATE[HY000] [2002] No such file or directory'); | ||
|
||
$backoff = new ExponentialBackoff(3); | ||
$backoff->retry(function (Exception $err = null) { | ||
throw new Exception('SQLSTATE[HY000] [2002] No such file or directory'); | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not maintain state and just do
$min << $attempt
as in our go code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He didn't do that on my request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please justify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessarily complex and difficult to understand for no apparent advantage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't be serious. If this is too complex (
2 ** $attempt * $min
), I wonder how we can build software.The obvious advantage is that no state is introduced without reason, which is then also changed in a getter.
The ipl should be an example of good coding practice. The current implementation is not.
The seemingly not so obvious advantage is that this does not fall apart when running asynchronously. Sure, PHP isn't ready to support everything concurrently yet, but PHP isn't the only programming language and there are fibers, async I/O (ReactPHP and the like) that already make this possible. When state like this one is introduced, concurrency should also be thought of. Even in PHP.
Also,
getWaitTime()
claims to return a proper wait time in the first line and then explains in the description that it actually does not. CallinggetWaitTime(n)
yields wrong results without having calledgetWaitTime(0..n-1)
first.Plus, when fixed, the actual backoff implementation would be properly testable.