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

Fix RetryMiddleware default exponential delay #2132

Merged
merged 4 commits into from Dec 7, 2019
Merged

Conversation

dluces
Copy link
Contributor

@dluces dluces commented Aug 28, 2018

RetryMiddleware::exponentialDelay was previously returning an integer (good) representing SECONDS of delay. The RetryMiddleware was putting that value directly into the delay option for the retried request. However, the docs say that the delay option should be in milliseconds.

If the RetryMiddleware was used without a custom delay function, the RetryMiddleware::exponentialDelay is used, meaning that the delay was going to be minimal by default.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

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?

@dluces
Copy link
Contributor Author

dluces commented Sep 7, 2018

@Nyholm the delay is not handled by the middleware but by a guzzle handler directly. The middleware sets the delay option and then sends off the request to the next handler. CurlHandler and other handlers check the delay option and do usleep($options['delay'] * 1000). That is, the middleware needs to set the delay in ms and not in seconds. I don't think we should test actual delays here as that's a different project (guzzle but the handler tests or a custom project if people use custom handlers). We need to test that our delay functions do what they should (return times in ms and not in seconds).

@Fuco1
Copy link

Fuco1 commented Jul 9, 2019

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.

@dluces
Copy link
Contributor Author

dluces commented Jul 10, 2019

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

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

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?

src/RetryMiddleware.php Show resolved Hide resolved
@Nyholm Nyholm added this to the 6.5.0 milestone Oct 30, 2019
@Nyholm
Copy link
Member

Nyholm commented Nov 5, 2019

Friendly update

dluces and others added 4 commits December 7, 2019 09:42
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.
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I rebased the PR and added a comment

@Nyholm Nyholm merged commit 1293c1b into guzzle:master Dec 7, 2019
TysonAndre added a commit to TysonAndre/guzzle that referenced this pull request Jan 23, 2020
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
TysonAndre added a commit to TysonAndre/guzzle that referenced this pull request Jan 23, 2020
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
gmponos pushed a commit that referenced this pull request May 18, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants