Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpClient] Fix buffering after calling AsyncContext::passthru()#47879
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
[HttpClient] Fix buffering after calling AsyncContext::passthru()#47879
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedOct 16, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
nicolas-grekas commentedOct 17, 2022
The attached patch doesn't look correct to me because it'd mean yielding a $chunk that should be skipped, since it belongs to a response that should be retried instead. Can you reproduce your scenario in a test case so that we can figure out another fix maybe? |
lubo13 commentedOct 17, 2022
Yes, sure it's not the correct one, it's just a dirty hack to try it out if we open the buffer do we receive the seekable stream? |
nicolas-grekas commentedOct 17, 2022
Not yet sorry. The test case would help a lot! |
lubo13 commentedOct 17, 2022
@nicolas-grekas the test has been added, I will waiting for your response |
8cc9e10 tob1d03d1Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(I added the fix to the PR)
b1d03d1 tob559f80Comparenicolas-grekas commentedOct 18, 2022
Thank you@lubo13. |
lubo13 commentedOct 18, 2022
@nicolas-grekas Thank you, could we apply this fix to 6.1 with a new bugfix version? Sorry, but I could find the tag, if you already have created such please share it. |
nicolas-grekas commentedOct 18, 2022
It's already merged up. Releases happen at the end of the month. |
lubo13 commentedOct 18, 2022
Okay, Thank you |
Uh oh!
There was an error while loading.Please reload this page.
Hi, guys, I think there is an issue with the RetryableHttpClient, let me explain to you what I've seen.
Request is executed into AsyncResponse::__construct
The initializer Closure executes AsyncResponse::passthru, there the AsyncContext and a new stream are created
The next step is yielding from passthruStream and there the stream is checked -
$r->stream->valid(). Here the stream is not a valid iterator, because the last retry attempt has already been made and the generator is closed.This leads the response body stream to be unseekable because the
$r->openBuffer()is not called. The content is left with a valuenulland whenAsyncResponse->toStream()is called fromPsr18Client::110the content with value null is bound to thecontent of StreamWrapper.After that this newly generated resource is passed to the
Psr17Factory->createStreamFromResource()and the resulted (unseekable body) stream is passed to the$response->withBody($body)ontoPsr18Client::121The result is that the Response body stream is not seekable and when I execute two times
(string) $response->getBody()after the first attempt the body is an empty string.Let me know what you think.
Thank you.