-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpClient] h2c support #36419
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
Comments
Can you try using the http_version option with an http URL? How does it behave? |
I tried using $client = new AmpHttpClient(['http_version' => '2.0']);
$client = new CurlHttpClient(['http_version' => '2.0']);
$options = [
'headers' => [
'Connection' => 'Upgrade, HTTP2-Settings',
'Upgrade' => 'h2c',
'HTTP2-settings' => 'AAMAAABkAARAAAAAAAIAAAAA',
]
];
$response = $client->request('GET', 'http://nghttp2.org');
$response->getStatusCode();
dd($response->getInfo()); Without the headers in
If I add the headers (curl does the same with --http2) so tell the server to upgrade to HTTP/2 amp client get an error
but curl hangs for a while but got back the response but didn't upgrade
In curl due to this https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpClient/CurlHttpClient.php#L146 HTTP/1.1 will be forced for 'http:' if I remove that the connection upgrades to HTTP/2
If I hardcode
|
Amp doesn't support h2 on http afaik, you should report there if you want to confirm. |
I don't know exactly it's done by curl internally I think it sends the same request again, but with http/2 without the upgrade headers. |
Sorry, I meant is any request after the first one using h2 directly? If yes is this enough a fix? |
I did some digging, but it seems to me it does not work the way it should. It seems it does not reuse the connection. When I create multiple requests with CLI curl it seems like this: $ curl --http2 -v nghttp2.org/robots.txt nghttp2.org/humans.txt
It does the upgrade once and reuses the connection. But the same with this code: $client = new CurlHttpClient(['http_version' => '2.0']);
$options = [
'headers' => [
'Connection' => 'Upgrade, HTTP2-Settings',
'Upgrade' => 'h2c',
'HTTP2-settings' => 'AAMAAABkAARAAAAAAAIAAAAA',
]
];
$responses = [];
foreach (['robots.txt', 'humans.txt'] as $i => $file) {
$uri = "http://nghttp2.org/$file";
$responses[] = $client->request('GET', $uri, 0 === $i ? $options : []);
}
/** @var ResponseInterface[] $responses */
foreach ($responses as $response) {
$response->getStatusCode();
dump($response->getInfo());
} Looks like this
Is this code should work the same way as the former? I think yes, but maybe I'm missing something or doing it wrong. But as I can't see a connection id or something like that so I checked with wireshark. the steam index is the same for all frames. But for php it seems like this the stream index is different so it creates 2 different connections for the requests. |
OK, thanks, fix is in #36422 Is this good enough? On non-SSL stream, there is no network overhead related to the SSL handshake, so that the multi-connections behavior is fine I think. The h2 negotiation adds no overhead, isn't it? |
Oh, thank you! The patch works great. I don't really know about the overhead, so you mean by that it does not worth it to add support for |
I think it's not worth it until proven otherwise yes. |
Ok, fair enough. Thank you very much for your help and the patch! |
…urlHttpClient only (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] fix HTTP/2 support on non-SSL connections - CurlHttpClient only | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36419 | License | MIT | Doc PR | - Commits ------- a5b884c [HttpClient] fix HTTP/2 support on non-SSL connections - CurlHttpClient only
@1ed Can you still reproduce the error you got with |
Nope, I get the same as you with the code I've posted earlier, after updating to #...
- Updating symfony/http-client-contracts (v2.0.1 => v2.1.1): Loading from cache
- Updating symfony/polyfill-php80 (v1.15.0 => v1.17.0): Loading from cache
- Updating amphp/amp (v2.4.2 => v2.4.4): Loading from cache
- Updating amphp/sync (v1.3.0 => v1.4.0): Loading from cache
- Updating amphp/http-client (v4.2.2 => v4.3.1): Loading from cache
- Updating symfony/http-client dev-master (5052db2 => c530027)
# ... @kelunik that means with an upgrade handler amphp can sand h2c requests too? Do you have an example? |
@1ed Not sure whether we'll support upgrades by default, but I've just opened a PR to allow for h2c if HTTP/2 is the only set protocol version, which requires only a small change: amphp/http-client#271 |
Uh oh!
There was an error while loading. Please reload this page.
Description
As far as I know it the HTTP Client component does not support HTTP/2 without TLS (aka. h2c). It could be useful for calling private endpoints without the need of certificate management but use the benefits of HTTP/2.
There are 2 methods to do it:
The php curl extension supports it via the
CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE
flag.The text was updated successfully, but these errors were encountered: