NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[HttpClient] Make retry strategy work again#53889

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:5.4fromNyholm:retry-strategy

Feb 14, 2024

Merged

[HttpClient] Make retry strategy work again#53889

nicolas-grekas merged 1 commit intosymfony:5.4fromNyholm:retry-strategy

Feb 14, 2024

Conversation

Copy link

Member

Nyholm commentedFeb 10, 2024•
edited
Loading

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix#53886
License	MIT

PR#53506 accidentally disabled the retry functionality. I reverted that PR and added a small test to make sure this does not happen again.

Thank you@ldebrouwer for reporting this.

FYI@nicolas-grekas @rmikalkenas, I will try to find an other solution tofix#52587. But I'll do that in a separate PR to get a quick merge on this one.

carsonbot added Status: Needs Review Bug HttpClient labels

Feb 10, 2024

carsonbot added this to the5.4 milestone

Feb 10, 2024

Copy link

MemberAuthor

Nyholm commentedFeb 11, 2024

Sorry, I could not find a fix for you@rmikalkenas. I only found "hacky" solutions..

I still suggest merging this and then reopen#52587

Copy link

Contributor

rmikalkenas commentedFeb 12, 2024•
edited
Loading

IMO it could be easily fixed by adjusting logic at

symfony/src/Symfony/Component/HttpClient/RetryableHttpClient.php

Line 63 in6986ab8

if ($context->getInfo('canceled') ||$chunk->isTimeout() ||null !==$chunk->getInformationalStatus()) {

With mine introduced fixisTimeout method will no longer throw aTransportExceptionInterface, therefore retry mechanism wont be called (looks like this part wasn't covered by tests and I didn't check it as well).

~~Removing previously mentioned$chunk->isTimeout() call from if block solves both issues - yours and mine.~~ Edit: it needs more changes, as we cannot blindly allow to retry all timeouts (idempotent methods etc)

In addition to that,Symfony\Contracts\HttpClient\ChunkInterface::isTimeout contract should be adjusted as well, since this method will never throw TransportException.

Copy link

MemberAuthor

Nyholm commentedFeb 13, 2024

it needs more changes, as we cannot blindly allow to retry all timeouts (idempotent methods etc)

Yeah. Im afraid so..

looks like this part wasn't covered by tests and I didn't check it as well

Correct. That is the biggest issue.
Since I am not 100% sure what changes needs to be done to fix both issues. I suggest to do a quick merge on this one so it can be released in next patch version. Then reopen and try to fix#52587