8000 [HttpClient] Fix decorating progress info in AsyncResponse by jderusse · Pull Request #38633 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpClient] Fix decorating progress info in AsyncResponse #38633

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
Merged
merged 1 commit into from
Oct 21, 2020
Merged

[HttpClient] Fix decorating progress info in AsyncResponse #38633

merged 1 commit into from
Oct 21, 2020

Conversation

jderusse
Copy link
Member
@jderusse jderusse commented Oct 19, 2020 •
edited
Loading

This PR reverts #38413, and send AsyncContext to onProgress callback.

@jderusse jderusse mentioned this pull request Oct 19, 2020
Closed
@jderusse jderusse changed the title Fix content not reported in TracableResponse decorated by AsyncResponse [HttpClient] Fix content not reported in TracableResponse decorated by AsyncResponse Oct 19, 2020
@derrabus
Copy link
Member
Q A
Branch? 5.x
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #38631
License MIT
Doc PR /

Can you write a test for this issue?

@BoShurik
Copy link
Contributor

With this PR I have response_content instead of response_json. Is it possible to get response_json?

@jderusse
Copy link
Member Author

Can you write a test for this issue?

Done.

With this PR I have response_content instead of response_json. Is it possible to get response_json?

We don't know how the response has been fetched (via getContent or toArray), thus we can't use response_json.
I've 2 solutions:

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 19, 2020 •
edited
Loading

revert #38413 and make TracableClient decorating RetryClient

that's an option.
We could then improve RetryableClient to make it report the retries in details (I'm thinking about merging the response_header and debug info of all retries in one)

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 19, 2020 •
edited
Loading

For the record, I skipped collecting the content consciously back then: to prevent collecting streamed responses (aka potentially long or even infinite content, think SSE).

@jderusse
Copy link
Member Author
jderusse commented Oct 19, 2020 •
edited
Loading

Ready for review.
Changed the implementation to revert Client decoraton and collect retries metadata

This is how it likes:
Screenshot from 2020-10-19 22-38-26

@derrabus derrabus added this to the 5.2 milestone Oct 19, 2020
nicolas-grekas
nicolas-grekas reviewed Oct 20, 2020
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Oct 20, 2020
View reviewed changes
@nicolas-grekas nicolas-grekas changed the title [HttpClient] Fix content not reported in TracableResponse decorated by AsyncResponse [HttpClient] Fix decorating progress info in AsyncResponse Oct 20, 2020
@jderusse jderusse force-pushed the fix-38631 branch 2 times, most recently from 51440f2 to 5c0d95d Compare October 20, 2020 09:55
@jderusse jderusse force-pushed the fix-38631 branch 2 times, most recently from 073b73b to 968c3a2 Compare October 20, 2020 12:39
@fabpot
Copy link
Member
fabpot commented Oct 21, 2020

Thank you @jderusse.

@fabpot fabpot merged commit 508ec9c into symfony:5.x Oct 21, 2020
@jderusse jderusse deleted the fix-38631 branch October 25, 2020 19:48
@fabpot fabpot mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr approved these changes

+1 more reviewer

@BoShurik BoShurik BoShurik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug HttpClient Status: Reviewed

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

[HttpClient] Missing response content in profiler after adding retryable http client

8 participants

@jderusse @derrabus @BoShurik @nicolas-grekas @fabpot @stof @chalasr @carsonbot
0