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] Add portable HTTP/2 implementation based on Amp's HTTP client #35115

Open
wants to merge 1 commit into
base: master
from

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 26, 2019

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

This PR provides an AmpHttpClient, which is an adapter between amphp/http-client and symfony/http-client-contracts.

This is an early experiment for now, but it works already on the happy path: I have a local h2-intensive script, and while it's slower than CurlHttpClient, this performs quite well!

This could provide a portable implementation of HTTP/2 \o/

/cc @kelunik FYI

Todo:

@nicolas-grekas nicolas-grekas added this to the next milestone Dec 26, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch from c81d7c1 to 40313c8 Dec 26, 2019
composer.json Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas changed the title [HttpClient] Add portable implementation based on Amp's HTTP client [HttpClient] Add portable HTTP/2 implementation based on Amp's HTTP client Dec 26, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch from 40313c8 to 17cee92 Dec 26, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch from 17cee92 to a6dde3d Dec 28, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch 6 times, most recently from 8b4f81a to 96b3fc5 Dec 28, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 30, 2019

Now with fixed redirection logic (not reusing the existing one from Amp because we want a different behavior that is consistent with curl+native)
I also added todo notes, which could be questions for you @kelunik :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch 3 times, most recently from 5a7a204 to f384258 Jan 3, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 3, 2020

The PR is now completed. It passes the full test suite, including the HTTP/2 PUSH tests. I'm pretty much impressed by how well this went.

This means we'll soon have two implementations fully compatible with HTTP/2 \o/
The Amp one is slower (at least without nghttp2 through FFI) but contrary to the curl-based one, it deals with Vary headers of pushed responses. Something that needs to be fixed on CurlHttpClient (but non-trivial IIRC).

Kudos @kelunik et al.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch 3 times, most recently from fe06889 to 7c9e60a Jan 3, 2020
nicolas-grekas added a commit that referenced this pull request Jan 4, 2020
…-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] fix casting responses to PHP streams

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

This patch is required to properly deal with casting responses to PHP streams.
This changes a public method, but we can't expect anyone to override it as it's totally internal.
Found when working on (and required by) #35115

Commits
-------

35c08ef [HttpClient] fix casting responses to PHP streams
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch 3 times, most recently from a9e63d4 to ff493dd Jan 4, 2020
if (null !== $tlsInfo = $stream->getTlsInfo()) {
foreach ($tlsInfo->getPeerCertificates() as $cert) {
$this->info['peer_certificate_chain'][] = openssl_x509_read($cert->toPem());
}

This comment has been minimized.

Copy link
@kelunik

kelunik Jan 5, 2020

Contributor

I think it makes sense to add getPeerCertificatesRaw to directly return the PEMs, as parsing them all results in memory leaks on older PHP versions and it's quite slow.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 5, 2020

Author Member

Or Certificate::toResource()?

This comment has been minimized.

Copy link
@kelunik

kelunik Jan 14, 2020

Contributor

Should $this->info['peer_certificate_chain'][] be always recorded or just if requested via an option?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 14, 2020

Author Member

it should be here if the option is set, then it might be set of the option is not set :)
so we can make this conditional.

This comment has been minimized.

Copy link
@kelunik

kelunik Jan 14, 2020

Contributor

That sounds good, because keeping it always enabled is quite some performance bottleneck. My suggestion is to move the pin calculation to Certificate, so we don't have to expose the resource. We could add TlsInfo::getPeerCertificate() to parse only the leaf certificate. This will keep the $this->info['peer_certificate_chain'][] collection a bit less efficient than it could be with exposing the resource, but I guess that's a fair tradeoff.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 14, 2020

Author Member

Works for me. Would you work on adding the pinning logic to the class? You can copy/paste from here of course!

This comment has been minimized.

Copy link
@kelunik

kelunik Jan 14, 2020

Contributor

I can do those, sure.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch 5 times, most recently from 7de90cd to 178bd5e Jan 5, 2020
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 force-pushed the nicolas-grekas:hc-amphp branch from 178bd5e to 59fae95 Jan 6, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 7, 2020

@kelunik I'm spotting that HTTP/2 is slow because of the LimitedConnectionPool, which limits to 6 streams in practice. How can I achieve 6 connections per host max with H2 enabled?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 8, 2020

Reported as amphp/http-client#246

which limits to 6 streams in practice. How can I achieve 6 connections per host max with H2 enabled

actually, this was not correct.
What happens is that 6 connections are opened, but then streams are not multiplexed.
When I force one single connection, even in H2, requests are run one after the other:

$client = new \Symfony\Component\HttpClient\AmpHttpClient(['http_version' => 2], null, 1);

for ($i = 0; $i < 379; ++$i) {
    $uri = "https://http2.akamai.com/demo/tile-$i.png";
    $responses[] = $client->request('GET', $uri);
}

foreach ($client->stream($responses) as $response => $chunk) {
    if ($chunk->isLast()) {
        echo '.';
        // a $response completed
    } else {
        // a $response's got network activity or timeout
    }
}
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch 2 times, most recently from b5c0b5a to da13724 Jan 14, 2020
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-amphp branch from da13724 to 80b1e44 Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.