Skip to content

Allow a callable parameter to decide if require is going to be retrie… #79

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

Merged
merged 7 commits into from
Jul 6, 2017
Merged
Changes from 2 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
29 changes: 28 additions & 1 deletion src/Plugin/RetryPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ final class RetryPlugin implements Plugin
*/
private $retry;

/**
* @var callable
*/
private $delay;

/**
* @var callable
*/
private $decider;

/**
* Store the retry counter for each request.
*
Expand All @@ -35,18 +45,30 @@ final class RetryPlugin implements Plugin
* @param array $config {
*
* @var int $retries Number of retries to attempt if an exception occurs before letting the exception bubble up.
* @var callable $decider A callback that gets a request and an exception to decide if we should retry this or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe: ... to decide after a failure whether the request should be retried.

* @var callable $delay A callback to return how many milliseconds we should wait before trying again.
Copy link
Contributor

Choose a reason for hiding this comment

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

we should document the arguments for this callback as well.

* }
*/
public function __construct(array $config = [])
{
$resolver = new OptionsResolver();
$resolver->setDefaults([
'retries' => 1,
'decider' => function (RequestInterface $request, Exception $e) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check the exception class and only do this when the exception indicates a network problem or a server error? 4xx responses or completely invalid requests don't need to be retried. (though we could want to retry dns errors)

if we make the logic more complicated, we should move this to a static public method of this class instead of the anonymous function, also to allow testing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not think we can add any logic here because it will break the existing behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree. we could argue that its more of a bugfix because retrying something that can't ever work out is pointless. but its quite possible we miss some situation where retrying could be successful, and thus we better change to something like that when we do a version 2. should we add an issue for that? or a todo comment in the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, adding sleeps is not a semantic change. How can program tell if it has been switched out by the scheduler? It is technically possible, but extremely unlikely, for the original code to have also slept there, if it was not allocated time.

Copy link
Member

@joelwurtz joelwurtz Jul 4, 2017

Choose a reason for hiding this comment

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

I don't think we should check the exception here, 4XX can have meaning to be rety in some scenario (like you have a pool of servers and you know that some files are present on some of them so you want to retry until you find the file)

That's why i propose the change in #77 so an user can choose on which behavior he wants exception (only 500, only 400 or both of them)

Then if an user choose only 500, 4XX response will never have exception and enter this callback, thus having a check here is not needed

Copy link
Contributor

Choose a reason for hiding this comment

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

@joelwurtz we agreed to keep it like this for BC reasons. can you please put your argument into #80 so that it does not get lost?

},
'delay' => function (RequestInterface $request, Exception $e, $retries) {
return ((int) pow(2, $retries - 1)) * 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be a public static function on this class. then we could unit test it.

},
]);
$resolver->setAllowedTypes('retries', 'int');
$resolver->setAllowedTypes('decider', 'callable');
$resolver->setAllowedTypes('delay', 'callable');
$options = $resolver->resolve($config);

$this->retry = $options['retries'];
$this->decider = $options['decider'];
$this->delay = $options['delay'];
}

/**
Expand All @@ -73,7 +95,12 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
throw $exception;
}

++$this->retryStorage[$chainIdentifier];
if (!call_user_func($this->decider, $request, $exception)) {
throw $exception;
}

$time = call_user_func($this->delay, $request, $exception, ++$this->retryStorage[$chainIdentifier]);
usleep($time);
Copy link
Contributor

Choose a reason for hiding this comment

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

this freezes the whole php process, even when doing async http requests.


// Retry in synchrone
$promise = $this->handleRequest($request, $next, $first);
Expand Down