Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Feb 10, 2024

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 to fix #52587. But I'll do that in a separate PR to get a quick merge on this one.

@Nyholm
Copy link
Member Author

Nyholm commented Feb 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

@rmikalkenas
Copy link
Contributor

rmikalkenas commented Feb 12, 2024

IMO it could be easily fixed by adjusting logic at

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

With mine introduced fix isTimeout method will no longer throw a TransportExceptionInterface, 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.

@Nyholm
Copy link
Member Author

Nyholm commented Feb 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

@nicolas-grekas
Copy link
Member

Thank you @Nyholm.

@nicolas-grekas nicolas-grekas merged commit 0ffaa0a into symfony:5.4 Feb 14, 2024
@Nyholm Nyholm deleted the retry-strategy branch February 15, 2024 00:15
@Nyholm
Copy link
Member Author

Nyholm commented Feb 15, 2024

Thank you for merging

This was referenced Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants