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.

Sign up for GitHub

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

Jump to bottom

[HttpClient] Fix error chunk creation in passthru #53506

Merged
merged 1 commit into from
Jan 26, 2024
Merged

[HttpClient] Fix error chunk creation in passthru #53506

merged 1 commit into from
Jan 26, 2024
+32 −7

Conversation

rmikalkenas
Copy link
Contributor

@rmikalkenas rmikalkenas commented Jan 11, 2024 •
edited
Loading

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

Timeout chunk should not be recreated as TransportException, because ErrorChunk losts information that it's timeout when passing thru.
Change solves issue mentioned at #52587

@carsonbot carsonbot added this to the 5.4 milestone Jan 11, 2024
@rmikalkenas rmikalkenas force-pushed the gh-52587 branch 2 times, most recently from 15a2106 to f67e49c Compare January 19, 2024 06:59
nicolas-grekas
nicolas-grekas reviewed Jan 23, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

It makes sense to remove this wrapper I agree.
Can this be tested somehow?
By curiosity, did you spot why GET vs POST behave differently?

@rmikalkenas
Copy link
Contributor Author

rmikalkenas commented Jan 24, 2024 •
edited
Loading

@nicolas-grekas

Can this be tested somehow?

It should be possible with MockHttpClient, but looks like timeout mock responses are broken. Returning MockResponse with empty string generator responds with 200 status code and empty content. ErrorChunk seems to be lost somewhere and instead FirstChunk gets created. Later on due to error chunk destructor exception is being thrown.
I will try to dig deeper what's wrong there and fix it -> #53625

By curiosity, did you spot why GET vs POST behave differently?

POST method errors is not intended to be retried by the strategy.
So under the hood this is what happens:

  • POST request fails and is being passed through the callback (same line I fixed in PR)
  • Then the retryable client simply wipes the passthru ($context->passthru();) and yields the remaining chunk
  • But the error chunk with timeout still is TransportException

With GET method is almost exactly the same, but on the last retry it explicitly wipes the passthru, therefore last retry does not have passthru callback and returns correct TimeoutException

@nicolas-grekas
Copy link
Member

Got it thanks.
For the tests, MockResponse doesn't support simulating a timeout during the request for now (see my comment in #53625).
But we do have a real http server available in the test suite, with an endpoint that behaves as we want (/timeout-header) so let's use it here?

@rmikalkenas
Copy link
Contributor Author

@nicolas-grekas updated. Failing test doesn't seem to be related with my changes

@rmikalkenas rmikalkenas force-pushed the gh-52587 branch 3 times, most recently from c8b1806 to 3e4a159 Compare January 26, 2024 07:21
@rmikalkenas @nicolas-grekas
[HttpClient] Fix error chunk creation in passthru

Verified

This commit was signed with the committer’s verified signature. The key has expired.
travi Matt Travi
GPG key ID: CB62529BDFD29894
Expired
Verified
Learn about vigilant mode
5e6d218
@nicolas-grekas
Copy link
Member

Thank you @rmikalkenas.

@nicolas-grekas nicolas-grekas merged commit c83c07e into symfony:5.4 Jan 26, 2024
This was referenced Jan 30, 2024
Merged
Merged
Merged
Merged
@ldebrouwer ldebrouwer mentioned this pull request Feb 9, 2024
Closed
Nyholm added a commit to Nyholm/symfony that referenced this pull request Feb 10, 2024
@Nyholm
[HttpClient] Make retry strategy work again

Unverified

This user has not yet uploaded their public signing key.
GPG key ID: 3EFD102DE8DE15DC
Learn about vigilant mode
Loading
Loading status checks…
b365e3a 
Reverts symfony#53506
@Nyholm Nyholm mentioned this pull request Feb 10, 2024
Merged
nicolas-grekas added a commit that referenced this pull request Feb 14, 2024
@nicolas-grekas
bug #53889 [HttpClient] Make retry strategy work again (Nyholm)
Loading
Loading status checks…
0ffaa0a 
This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[HttpClient] Make retry strategy work again

| 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.

Commits
-------

9a5797d [HttpClient] Make retry strategy work again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas

Assignees
No one assigned
Labels
Bug HttpClient Status: Needs Review
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rmikalkenas @nicolas-grekas @carsonbot