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

Nyholm · 2017-07-01T17:27:47Z

…d or not.

Also allow retry backoff

This will fix #78

It is a minor breaking change here. Instead of retry immediately we wait a second. I do not consider it a BC break though..

…d or not. Also allow retry backoff

dbu

i added some suggestions for the phpdoc on the new callables.

i have one larger worry with this code: we freeze the whole php process even when doing async requests.
when doing multiple requests that fail, we will unconditionally delay for each of the requests. i wonder if we could instead work with a timestamp. like recording that response was received at time X and if we get here we wait until X+$time. for a single request, this will result in the same delay, but if we sent out a bunch of requests we will only sleep once if we follow that logic.

dbu · 2017-07-03T06:13:42Z

src/Plugin/RetryPlugin.php

@@ -35,18 +45,30 @@
     * @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.


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

dbu · 2017-07-03T06:15:22Z

src/Plugin/RetryPlugin.php

@@ -35,18 +45,30 @@
     * @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.
+     *     @var callable $delay A callback to return how many milliseconds we should wait before trying again.


we should document the arguments for this callback as well.

dbu · 2017-07-03T06:22:09Z

src/Plugin/RetryPlugin.php

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


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

dbu · 2017-07-03T06:26:00Z

src/Plugin/RetryPlugin.php

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


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.

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

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?

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.

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

@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?

dbu · 2017-07-03T06:26:21Z

src/Plugin/RetryPlugin.php

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


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

Nyholm · 2017-07-03T07:04:57Z

when doing multiple requests that fail, we will unconditionally delay for each of the requests. i wonder if we could instead work with a timestamp. like recording that response was received at time X and if we get here we wait until X+$time. for a single request, this will result in the same delay, but if we sent out a bunch of requests we will only sleep once if we follow that logic.

Does this makes sense? Or I mean, does it work?

If one do two async requests that both will fail. At some point request A will fail, we check the timestamp and we sleep. Request B will not fail until Request A has been retried, right?
When Request B fails we take the new timestamp..

It would only make sense if we slept between:

$promise = $this->handleRequest($request, $next, $first);
return $promise->wait();

Or am I wrong?

dbu · 2017-07-03T08:36:37Z

Request B will not fail until Request A has been retried, right?

i guess you are correct. can we find a timestamp coming from the curl library that was doing the actual async requests? because those should happen in actual threads... if we can't i guess this has to be good enough.

Nyholm · 2017-07-03T08:52:15Z

We can't I'm afraid.. Since we do not know what client they are using.

fbourigault · 2017-07-03T09:56:28Z

It would only make sense if we slept between:

$promise = $this->handleRequest($request, $next, $first);
return $promise->wait();

You have to sleep before $promise->wait() but you can also sleep before $promise = $this->handleRequest($request, $next, $first);. Or you can do something like:

$time = microtime(true);
$promise = $this->handleRequest($request, $next, $first);
usleep($delay + ($time - microtime(true)) * 1000);
return $promise->wait();

This will remove the $promise = $this->handleRequest($request, $next, $first); execution time from the delay to sleep. It can save a really few µs.

dbu · 2017-07-03T13:19:23Z

i don't think saving microseconds is worth it. its an error case and we wait for a second or similar, which is magnitudes longer. the only problem i see is multiple waits of 1 (or several) seconds each.

Nyholm · 2017-07-04T12:58:22Z

I do not think there is a solution to the problem you highlight. I updated the PR with a change log. I think this is ready to merge

dbu

lets squash and merge

do we need an update in the documentation? should we support this in the HttplugBundle configuration?

joelwurtz · 2017-07-04T20:15:20Z

For the problem of having to wait here i recently come to an idea to be full asynchronous here instead of using the wait, but it will require some work :).

First we need a custom Promise implementation which we cannot resolve and we will return this promise in this plugin :

$promise = new CustomPromiseImplementation();

...

return $promise;

Then when we receive the response from the upstream promise, we can resolve our custom promise with response if it's OK

$promise = new CustomPromiseImplementation();

$next($request)->then(function (ResponseInterface $response) use ($request, $chainIdentifier) {
             if (array_key_exists($chainIdentifier, $this->retryStorage)) {
                 unset($this->retryStorage[$chainIdentifier]);
             }

             $promise->resolve($response);
 
             return $response;
         }
...

return $promise;

And then when it fails we can resend a new async request and resolve or make it failed (when too many retries) our custom promise just like before.

If nobody is up to do that i may throw a PR for this in the summer to resolve this but i have so many things to do right now that not sure if i will have the time for it so feel free to do it.

joelwurtz · 2017-07-04T20:17:31Z

Also using something like this mean we can reuse this principle in other code and drop the wait method from our interface (for a 2.0 version of course)

dbu · 2017-07-05T06:25:51Z

interesting idea, joel. i suggest we merge this anyways and see for other solutions. if there will be a BC break involved, we can do a 2.0 version. is that ok for you?

sagikazarmark · 2017-07-05T20:17:06Z

@joelwurtz I remember you wanted to use async-interop. Is this something that they do as well. If not, what's your current point of view on this question: should we still lean towards async-interop?

sagikazarmark · 2017-07-05T20:17:34Z

CHANGELOG.md

+
+### Changed
+
+- The `RetryPlugin` does now wait between retries. To disable/change this feature you must write something like: 


Are you sure? 'It does now waits'?

sagikazarmark · 2017-07-05T20:18:40Z

spec/Plugin/RetryPluginSpec.php

@@ -101,4 +101,12 @@ function it_does_not_keep_history_of_old_failure(RequestInterface $request, Resp
        $this->handleRequest($request, $next, function () {})->shouldReturnAnInstanceOf('Http\Client\Promise\HttpFulfilledPromise');
        $this->handleRequest($request, $next, function () {})->shouldReturnAnInstanceOf('Http\Client\Promise\HttpFulfilledPromise');
    }
+
+    function it_has_a_good_default_delay(RequestInterface $request, Exception\HttpException $exception)


I'm sorry, but what does good mean? 😂 Dat software specification 😂

Haha. You know... not "bad".
I'll update :)

sagikazarmark · 2017-07-05T20:22:28Z

src/Plugin/RetryPlugin.php

+     */
+    public static function defaultDelay(RequestInterface $request, Exception $e, $retries)
+    {
+        return ((int) pow(2, $retries)) * 1000;


I guess $retries can only be a positive integer or 0, so why the int casting?

Not sure to be honest. Just copied this from guzzle.

Nyholm · 2017-07-06T07:55:10Z

I've updated the PR

joelwurtz · 2017-07-06T10:01:16Z

Let's go merge this, my comment is not a blocker, and can be done later.

@sagikazarmark async-interop is dead so don't know actually, i would like to changing the async interface to be used with coroutines instead (like async / await in other languages, see php amp for the details), but may be a bit soon for the php community.

bendavies · 2017-09-05T09:04:22Z

@Nyholm just started using this. the default defaultDelay doesn't seem like a great default to me.

with 5 retries, the plugin retries pretty much instantly from 0.001 to 0.032 seconds. no delay at all really. Clearly I can define my own, delay, but maybe the default could be a bit more ...delaying?

Nyholm · 2017-09-05T09:11:52Z

Hm, I guess you are correct. We should update our defaults. Could you make a suggestion in a PR?

bendavies · 2017-09-05T09:12:41Z

sure.

Nyholm added 2 commits July 1, 2017 19:26

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

fdd6afa

…d or not. Also allow retry backoff

Applied changes from StyleCI

cb37fad

dbu reviewed Jul 3, 2017

View reviewed changes

Some fixes

98941f5

Nyholm mentioned this pull request Jul 3, 2017

Improve the default callable in the RetryPlugin #80

Closed

Applied changes from StyleCI

5e80c64

Updated changelog

bd9a5a4

dbu approved these changes Jul 4, 2017

View reviewed changes

sagikazarmark reviewed Jul 5, 2017

View reviewed changes

Nyholm added 2 commits July 6, 2017 09:27

Stuff

237561d

Merge branch 'master' into nyholm-callable

84bbd63

Nyholm merged commit c87566a into master Jul 6, 2017

Nyholm deleted the nyholm-callable branch July 10, 2017 21:33

bendavies mentioned this pull request Sep 5, 2017

RFC: update retry plugin backoff default to be more sensible #86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Nyholm commented Jul 1, 2017

dbu left a comment

dbu Jul 3, 2017

dbu Jul 3, 2017

dbu Jul 3, 2017

dbu Jul 3, 2017

Nyholm Jul 3, 2017

dbu Jul 3, 2017

GrahamCampbell Jul 3, 2017

joelwurtz Jul 4, 2017 •

edited

Loading

dbu Jul 5, 2017

dbu Jul 3, 2017

Nyholm commented Jul 3, 2017

dbu commented Jul 3, 2017

Nyholm commented Jul 3, 2017

fbourigault commented Jul 3, 2017

dbu commented Jul 3, 2017

Nyholm commented Jul 4, 2017

dbu left a comment •

edited

Loading

joelwurtz commented Jul 4, 2017 •

edited

Loading

joelwurtz commented Jul 4, 2017 •

edited

Loading

dbu commented Jul 5, 2017

sagikazarmark commented Jul 5, 2017

sagikazarmark Jul 5, 2017

Nyholm Jul 5, 2017

sagikazarmark Jul 5, 2017

Nyholm Jul 5, 2017

sagikazarmark Jul 5, 2017

Nyholm Jul 5, 2017

Nyholm commented Jul 6, 2017

joelwurtz commented Jul 6, 2017

bendavies commented Sep 5, 2017 •

edited

Loading

Nyholm commented Sep 5, 2017

bendavies commented Sep 5, 2017


		### Changed

		- The `RetryPlugin` does now wait between retries. To disable/change this feature you must write something like:

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

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

Conversation

Nyholm commented Jul 1, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelwurtz Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Jul 3, 2017

dbu commented Jul 3, 2017

Nyholm commented Jul 3, 2017

fbourigault commented Jul 3, 2017

dbu commented Jul 3, 2017

Nyholm commented Jul 4, 2017

dbu left a comment • edited Loading

Choose a reason for hiding this comment

joelwurtz commented Jul 4, 2017 • edited Loading

joelwurtz commented Jul 4, 2017 • edited Loading

dbu commented Jul 5, 2017

sagikazarmark commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Jul 6, 2017

joelwurtz commented Jul 6, 2017

bendavies commented Sep 5, 2017 • edited Loading

Nyholm commented Sep 5, 2017

bendavies commented Sep 5, 2017

joelwurtz Jul 4, 2017 •

edited

Loading

dbu left a comment •

edited

Loading

joelwurtz commented Jul 4, 2017 •

edited

Loading

joelwurtz commented Jul 4, 2017 •

edited

Loading

bendavies commented Sep 5, 2017 •

edited

Loading