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

Introduce Backoff implementation with exponential strategy #50

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 176 additions & 0 deletions src/ExponentialBackoff.php
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;
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Please justify.

Copy link
Member

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

Copy link
Member Author

@yhabteab yhabteab Sep 13, 2023

Choose a reason for hiding this comment

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

php > $min = 100;
php > var_dump($min*2, $min << 1);
php shell code:1:
int(200)
php shell code:1:
int(200)
php > var_dump($min*2*2, $min << 2);
php shell code:1:
int(400)
php shell code:1:
int(400)
php > var_dump($min*2*2*2, $min << 3);
php shell code:1:
int(800)
php shell code:1:
int(800)
...

Copy link
Member

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

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. Calling getWaitTime(n) yields wrong results without having called getWaitTime(0..n-1) first.
Plus, when fixed, the actual backoff implementation would be properly testable.


/**
* 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
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The 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());
}
}
78 changes: 78 additions & 0 deletions tests/ExponentialBackoffTest.php
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()
Copy link
Member

Choose a reason for hiding this comment

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

Should also test the waiting time for attempt n, where 0..n-1 has not yet been called.

{
$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');
});
}
}