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.

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

[HttpClient] Don't read from the network faster than the CPU can deal with #35223

Merged
merged 1 commit into from Jan 6, 2020

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 6, 2020

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

Something I spotted while working on #35115: both the curl and native clients don't play well with heavily compressed HTTP streams: they decompress faster than userland can process chunks.

The attached patch moves the decompression logic to the chunk generator. This means internally we only deal with raw compressed chunks, and they are decompressed only when passing the value to userland.

@@ -113,7 +113,7 @@ public function request(string $method, string $url, array $options = []): Respo
$url = implode('', $url);

if (!isset($options['normalized_headers']['user-agent'])) {
$options['normalized_headers']['user-agent'][] = $options['headers'][] = 'User-Agent: Symfony HttpClient/Curl';
$options['headers'][] = 'User-Agent: Symfony HttpClient/Curl';

This comment has been minimized.

Copy link
@stof

stof Jan 6, 2020

Member

is this change related to this gzip issue ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 6, 2020

Author Member

not directly (I spotted this was not required while working on the patch)

nicolas-grekas added a commit that referenced this pull request Jan 6, 2020
…PU can deal with (nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

[HttpClient] Don't read from the network faster than the CPU can deal with

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

Something I spotted while working on #35115: both the curl and native clients don't play well with heavily compressed HTTP streams: they decompress faster than userland can process chunks.

The attached patch moves the decompression logic to the chunk generator. This means internally we only deal with raw compressed chunks, and they are decompressed only when passing the value to userland.

Commits
-------

ac3d77a [HttpClient] Don't read from the network faster than the CPU can deal with
@nicolas-grekas nicolas-grekas merged commit ac3d77a into symfony:4.3 Jan 6, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:hc-bomb branch Jan 6, 2020
}
if ('' !== $chunk && null !== $response->content && \strlen($chunk) !== fwrite($response->content, $chunk)) {
$multi->handlesActivity[$j] = [null, new TransportException('Failed writing %d bytes to the response buffer.', \strlen($chunk))];

This comment has been minimized.

Copy link
@TiGR

TiGR Jan 7, 2020

Contributor

@nicolas-grekas I think sprintf is supposed to be here.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 7, 2020

Author Member

correct :) would you mind sending a PR?

This comment has been minimized.

Copy link
@TiGR

TiGR Jan 7, 2020

Contributor

Done #35250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.