Fix RetryMiddleware default exponential delay#2132
Conversation
Nyholm
left a comment
There was a problem hiding this comment.
Maybe Im reading this wrong. But this does not delay anything.
Could you write a test that make sure we do actually sleep between retries?
|
@Nyholm the delay is not handled by the middleware but by a guzzle handler directly. The middleware sets the |
|
Can we get more attention from maintainers to this issue? The middleware by default does "not" back off at all since it returns 2^1/2/3/4/5 which is <100 in milliseconds now. This bombards the servers with tens of requests per second. |
|
I've just updated this PR's branch with latest master. I still think this should not be testing the actual delay mechanism as nothing related to that has been changed. Instead, it only changes how the delay time is calculated, @Nyholm |
Nyholm
left a comment
There was a problem hiding this comment.
The issue is the units..
The option delay expects a delay in milliseconds (ms). We are retuning 0,1,2,4,8 ms which is correct.
This PR is increasing the delay with a factor 1000,
Could you do another rebase please?
|
Friendly update |
RetryMiddleware::exponentialDelay was being used as-is to set the delay option in the retry middleware. However, the option requires milliseconds and this method was returning seconds.
RetryMiddleware::exponentialDelay was being used as-is to set the delay option in the retry middleware. However, the option requires milliseconds and this method was returning seconds.
Nyholm
left a comment
There was a problem hiding this comment.
I rebased the PR and added a comment
Related to guzzle#2132 A fraction of use cases will expect low timeouts or no timeouts. Make it clearer that those applications should override the defaults of the middleware if upgrading to 6.5.0
Related to guzzle#2132 A fraction of use cases will expect low timeouts or no timeouts. Make it clearer that those applications should override the defaults of the middleware if upgrading to 6.5.0
Related to #2132 A fraction of use cases will expect low timeouts or no timeouts. Make it clearer that those applications should override the defaults of the middleware if upgrading to 6.5.0
RetryMiddleware::exponentialDelaywas previously returning an integer (good) representing SECONDS of delay. TheRetryMiddlewarewas putting that value directly into thedelayoption for the retried request. However, the docs say that thedelayoption should be in milliseconds.If the
RetryMiddlewarewas used without a custom delay function, theRetryMiddleware::exponentialDelayis used, meaning that the delay was going to be minimal by default.