retry http 5xx status as well

[dbu]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Related tickets	fixes #126
Documentation	php-http/documentation#246
License	MIT

What's in this PR?

Add retry for responses with a server error status code.

Checklist

• Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

TODO

• Rebase and cleanup when rename decider to exception_decider to make room for response_decider in version 2 #142 has been merged to master and master to 2.x

[Nyholm]

Good, I like this

[dbu]

i'll backport the rename to 1.9 with deprecation, and remove the BC code here

[dbu]

rebased and cleaned up. this is now ready for review again.

[Nyholm]

Good, just some minor changes

[sagikazarmark]

Are you sure this is a good idea? 5xx generally means everything is broken and the server tells the client to leave them alone. Imagine if everyone retried requests upon an 500 error: poor server wouldn't have any chance to recover.

[Nyholm]

What other request should you retry? 4xx means that the request itself is wrong.

[sagikazarmark]

Not necessarily. 404 could mean a resource is not ready yet, but will be at some point.

I guess retry from application and retry from http client point of view mean two different things.

[GrahamCampbell]

Not necessarily. 404 could mean a resource is not ready yet, but will be at some point.

People can always configure reties themselves for that case though. For example, I do this when hitting the GitHub API, because they have a lag due to SQL replication.

[dbu]

to me retrying 5xx seems reasonable, might be a temporary lapse in server communication or a database deadlock or whatnot.

we can also by default not retry any response codes, if we think that is better. just having the configuration option has value already. what do we do?

[Nyholm]

If you are using the RetryPlugin you are assuming that at least some request should be retried.

I understand both arguments. I guess it depends on what kind of services you are using. I dont mind retrying my 500 requests when Im requesting github. But I may reconsider if I querying my own "easy to dos"-servers.

I vote for a merge.

[dbu]

But I may reconsider if I querying my own "easy to dos"-servers.

we can add a warning in the documentation. there is the exponential backoff, so there is not that much requests to be expected.

[dbu]

added documentation PR and realized i only want to retry codes from 500 to < 600. some systems (ab)use higher http status codes for custom things and we don't want to accidentally interact with those by default.

[GrahamCampbell]

Looks good. 👍

[sagikazarmark]

LGTM, thanks @dbu

[gmponos]

1. On this PR you are altering the default behavior of the plugin but a developer can set his own if he wishes to right? For what it is worth as a consumer developer I would never use this plugin without setting my custom callback functions... Therefore I would prefer if the default is null.
2. For the reason above I would prefer if the callback were allowed to be also null instead of having to pass a callback function with just a return...
3. For me this line return !$e instanceof HttpException || $e->getCode() >= 500 && $e->getCode() < 600; is confusing... It might make sense in the cases where you have a PluginClient with the ErrorPlugin inside of it... if the PluginClient becomes a PSR compatible client then according to PSR

A Client MUST NOT treat a well-formed HTTP request or HTTP response as an error condition. For example, response status codes in the 400 and 500 range MUST NOT cause an exception and MUST be returned to the Calling Library as normal.

So I don't understand how this $e->getCode() >= 500 && $e->getCode() < 600 can be fullfilled.. Because it is confusing I would really love if someone has the time to represent a full example with this. Maybe on a test...

On another topic please rethink the usage of ErrorPlugin with PluginClient on v2. On a discussion we had on chat if we initialize on v2 a PluginClient with the ErrorPlugin it wont be compatible with PSR-18

[gmponos]

For what it is worth as a consumer developer I would never use this plugin without setting my custom callback functions..

Elaborating this.. Using guzzle retry middleware... I only tend to retry 503 cases and also cases that the request was not even sent.. (status code 0 because of cURL)... Or cases that I got a response and I must look inside of the response...

[dbu]

allowing to pass null makes the code more complicated. and the plugin is not active by default, so it can be used if wanted but is not needed. the exception part of the plugin indeed only makes sense with the error plugin - this is also explicitly mentioned in the documentation to avoid confusion

following our slack discussion, i added a note to the ErrorPlugin that indeed it is violating PSR-18. it can still be used when writing an application and wanting exceptions, but must not be used with third party libraries.

[gmponos]

allowing to pass null makes the code more complicated.

and the plugin is not active by default

I understand these part.. None the less the null has to be taken in consideration along with the other parts that I said...

I saw also the documentation.. but I don't believe that the documentation here saves the day.. in overall, for what my opinion is worth it, I am not excited very much about these changes... :)