Skip to content

[HttpClient] Fix content swallowed by AsyncClient initializer #39228

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
Dec 10, 2020

Conversation

jderusse
Copy link
Member

Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

I'm not sure if it should be fixed in RetryableHttpClient or AsycClient.
The issue is: when the Strategy needs the body to take a decision BUT decide to NOT retry the request, the content is "lost"

In fact, when the first chunk is yield, the AsyncResponse's initializer is stopped, and nothing consume the remaining chunks. Moreover, because the passthru were disabled before yielding the first chunk in RetryableHttpClient, the callback is never called again to yield the remaining content.

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.

Thank you @jderusse for your work on this.

@nicolas-grekas
Copy link
Member

As discussed on Slack, this should be fixed in AsyncResponse instead.

@jderusse jderusse force-pushed the fix-retry-content branch 2 times, most recently from 785fbe8 to bfc4b10 Compare November 30, 2020 13:24
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've tested this and I can confirm that it fixes the issue

@jderusse jderusse force-pushed the fix-retry-content branch 2 times, most recently from 1df90ba to b454235 Compare December 10, 2020 12:11
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.

LGTM after some changes.

@nicolas-grekas
Copy link
Member

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit 359bcc5 into symfony:5.2 Dec 10, 2020
@jderusse jderusse deleted the fix-retry-content branch December 10, 2020 14:16
@fabpot fabpot mentioned this pull request Dec 18, 2020
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