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] h2c support #36419

Closed
1ed opened this issue Apr 11, 2020 · 13 comments
Closed

[HttpClient] h2c support #36419

1ed opened this issue Apr 11, 2020 · 13 comments

Comments

@1ed
Copy link
Contributor

1ed commented Apr 11, 2020

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:

$ curl --http2 -I http://nghttp2.org

HTTP/1.1 101 Switching Protocols
Connection: Upgrade
Upgrade: h2c

HTTP/2 200 
date: Fri, 10 Apr 2020 23:53:40 GMT
content-type: text/html
last-modified: Fri, 15 Nov 2019 14:36:38 GMT
etag: "5dceb7f6-19d8"
accept-ranges: bytes
content-length: 6616
x-backend-header-rtt: 0.001996
server: nghttpx
via: 2 nghttpx
alt-svc: h3-23=":4433"; ma=3600
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
$ curl --http2-prior-knowledge -I http://nghttp2.org

HTTP/2 200 
date: Fri, 10 Apr 2020 23:53:42 GMT
content-type: text/html
last-modified: Fri, 15 Nov 2019 14:36:38 GMT
etag: "5dceb7f6-19d8"
accept-ranges: bytes
content-length: 6616
x-backend-header-rtt: 0.001454
server: nghttpx
via: 2 nghttpx
alt-svc: h3-23=":4433"; ma=3600
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
x-content-type-options: nosniff

The php curl extension supports it via the CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE flag.

@nicolas-grekas
Copy link
Member

Can you try using the http_version option with an http URL? How does it behave?

@1ed
Copy link
Contributor Author

1ed commented Apr 11, 2020

I tried using dev-master for symfony/http-client

$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 $options itt falls back to HTTP/1.1 for both clients:

  "response_headers" => array:14 [
    0 => "HTTP/1.1 200 OK"
    1 => "Date: Sat, 11 Apr 2020 06:38:35 GMT"
    2 => "Content-Type: text/html"
    3 => "Last-Modified: Fri, 15 Nov 2019 14:36:38 GMT"
    4 => "Etag: "5dceb7f6-19d8""
    5 => "Accept-Ranges: bytes"
    6 => "Content-Length: 6616"
    7 => "X-Backend-Header-Rtt: 0.003255"
    8 => "Server: nghttpx"
    9 => "Via: 2 nghttpx"
    10 => "alt-svc: h3-23=":4433"; ma=3600"
    11 => "x-frame-options: SAMEORIGIN"
    12 => "x-xss-protection: 1; mode=block"
    13 => "x-content-type-options: nosniff"
  ]
// ...

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

PHP Fatal error:  Uncaught Error: Call to a member function close() on null in .../vendor/amphp/http-client/src/Connection/Http1Connection.php:229
Stack trace:
#0 [internal function]: Amp\Http\Client\Connection\Http1Connection->Amp\Http\Client\Connection\{closure}()
#1 .../vendor/amphp/amp/lib/Coroutine.php(118): Generator->send()
#2 .../vendor/amphp/amp/lib/Internal/Placeholder.php(149): Amp\Coroutine->Amp\{closure}()
#3 .../vendor/amphp/amp/lib/Deferred.php(52): class@anonymous->resolve()
#4 .../vendor/amphp/byte-stream/lib/ResourceInputStream.php(101): Amp\Deferred->resolve()
#5 .../vendor/amphp/amp/lib/Loop/NativeDriver.php(201): Amp\ByteStream\ResourceInputStream::Amp\ByteStream\{closure}()
#6 .../vendor/a in .../vendor/amphp/http-client/src/Connection/Http1Connection.php on line 229

but curl hangs for a while but got back the response but didn't upgrade

  "response_headers" => array:3 [
    0 => "HTTP/1.1 101 Switching Protocols"
    1 => "Connection: Upgrade"
    2 => "Upgrade: h2c"
  ]
// ...

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

    "response_headers" => array:17 [
    0 => "HTTP/1.1 101 Switching Protocols"
    1 => "Connection: Upgrade"
    2 => "Upgrade: h2c"
    3 => "HTTP/2 200 "
    4 => "date: Sat, 11 Apr 2020 06:46:42 GMT"
    5 => "content-type: text/html"
    6 => "last-modified: Fri, 15 Nov 2019 14:36:38 GMT"
    7 => "etag: "5dceb7f6-19d8""
    8 => "accept-ranges: bytes"
    9 => "content-length: 6616"
    10 => "x-backend-header-rtt: 0.002924"
    11 => "server: nghttpx"
    12 => "via: 2 nghttpx"
    13 => "alt-svc: h3-23=":4433"; ma=3600"
    14 => "x-frame-options: SAMEORIGIN"
    15 => "x-xss-protection: 1; mode=block"
    16 => "x-content-type-options: nosniff"
  ]
// ...

If I hardcode $curlopts[CURLOPT_HTTP_VERSION] = CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE; (the same as adding --http2-prior-knowledge to the curl cli) I got HTTP/2 even without addig the $toptions

  "response_headers" => array:14 [
    0 => "HTTP/2 200 "
    1 => "date: Sat, 11 Apr 2020 06:50:14 GMT"
    2 => "content-type: text/html"
    3 => "last-modified: Fri, 15 Nov 2019 14:36:38 GMT"
    4 => "etag: "5dceb7f6-19d8""
    5 => "accept-ranges: bytes"
    6 => "content-length: 6616"
    7 => "x-backend-header-rtt: 0.00356"
    8 => "server: nghttpx"
    9 => "via: 2 nghttpx"
    10 => "alt-svc: h3-23=":4433"; ma=3600"
    11 => "x-frame-options: SAMEORIGIN"
    12 => "x-xss-protection: 1; mode=block"
    13 => "x-content-type-options: nosniff"
  ]
  // ...

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 11, 2020

Amp doesn't support h2 on http afaik, you should report there if you want to confirm.
That L146: how does a second request look like after the first responded with an upgrade?

@1ed
Copy link
Contributor Author

1ed commented Apr 11, 2020

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.

@nicolas-grekas
Copy link
Member

Sorry, I meant is any request after the first one using h2 directly? If yes is this enough a fix?
Otherwise I'd suggest a new syntax to force a specific version. e.g. !2
Up for up PR, one way or another (or both)?

@1ed
Copy link
Contributor Author

1ed commented Apr 11, 2020

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
// ...
* Expire in 14 ms for 1 (transfer 0x560f6c51a740)
*   Trying 139.162.123.134...
* TCP_NODELAY set
* Expire in 149978 ms for 3 (transfer 0x560f6c51a740)
* Expire in 200 ms for 4 (transfer 0x560f6c51a740)
*   Trying 2400:8902::f03c:91ff:fe69:a454...
* TCP_NODELAY set
* Expire in 149978 ms for 3 (transfer 0x560f6c51a740)
* Immediate connect fail for 2400:8902::f03c:91ff:fe69:a454: Network is unreachable
* Connected to nghttp2.org (139.162.123.134) port 80 (#0)
> GET /robots.txt HTTP/1.1
> Host: nghttp2.org
> User-Agent: curl/7.64.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> 
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c
* Received 101
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=336
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
< HTTP/2 200 
< date: Sat, 11 Apr 2020 08:31:56 GMT
< content-type: text/plain
< last-modified: Fri, 15 Nov 2019 14:36:38 GMT
< etag: "5dceb7f6-3e"
< accept-ranges: bytes
< content-length: 62
< x-backend-header-rtt: 0.002798
< server: nghttpx
< via: 2 nghttpx
< alt-svc: h3-23=":4433"; ma=3600
< x-frame-options: SAMEORIGIN
< x-xss-protection: 1; mode=block
< x-content-type-options: nosniff
< 
User-agent: *
Disallow: 

Sitemap: //nghttp2.org/sitemap.xml 
* Connection #0 to host nghttp2.org left intact
* Expire in 0 ms for 6 (transfer 0x560f6c51a740)
* Found bundle for host nghttp2.org: 0x560f6c519c10 [can multiplex]
* Re-using existing connection! (#0) with host nghttp2.org
* Connected to nghttp2.org (139.162.123.134) port 80 (#0)
* Using Stream ID: 3 (easy handle 0x560f6c51a740)
> GET /humans.txt HTTP/2
> Host: nghttp2.org
> User-Agent: curl/7.64.0
> Accept: */*
> 
< HTTP/2 404 
< date: Sat, 11 Apr 2020 08:31:57 GMT
< content-type: text/plain; charset=utf-8
< content-length: 9
< x-backend-header-rtt: 0.001403
< server: nghttpx
< via: 2 nghttpx
< alt-svc: h3-23=":4433"; ma=3600
< x-frame-options: SAMEORIGIN
< x-xss-protection: 1; mode=block
< x-content-type-options: nosniff
< 
* Connection #0 to host nghttp2.org left intact

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
^ array:44 [
  "response_headers" => array:17 [
    0 => "HTTP/1.1 101 Switching Protocols"
    1 => "Connection: Upgrade"
    2 => "Upgrade: h2c"
    3 => "HTTP/2 200 "
    4 => "date: Sat, 11 Apr 2020 08:42:13 GMT"
    5 => "content-type: text/plain"
    6 => "last-modified: Fri, 15 Nov 2019 14:36:38 GMT"
    7 => "etag: "5dceb7f6-3e""
    8 => "accept-ranges: bytes"
    9 => "content-length: 62"
    10 => "x-backend-header-rtt: 0.00276"
    11 => "server: nghttpx"
    12 => "via: 2 nghttpx"
    13 => "alt-svc: h3-23=":4433"; ma=3600"
    14 => "x-frame-options: SAMEORIGIN"
    15 => "x-xss-protection: 1; mode=block"
    16 => "x-content-type-options: nosniff"
  ]
  "http_code" => 200
  "error" => null
  "canceled" => false
  "http_method" => "GET"
  "user_data" => null
  "start_time" => 1586594532.8838
  "redirect_url" => null
  "url" => "http://nghttp2.org/robots.txt"
  "content_type" => "text/plain"
  "header_size" => 450
  "request_size" => 306
  "filetime" => -1
  "ssl_verify_result" => 0
  "redirect_count" => 0
  "total_time" => 0.696832
  "namelookup_time" => 0.024732
  "connect_time" => 0.387726
  "pretransfer_time" => 0.387954
  "size_upload" => 0.0
  "size_download" => 62.0
  "speed_download" => 89.0
  "speed_upload" => 0.0
  "download_content_length" => 62.0
  "upload_content_length" => -1.0
  "starttransfer_time" => 0.695197
  "redirect_time" => 0.0
  "primary_ip" => "139.162.123.134"
  "certinfo" => []
  "primary_port" => 80
  "local_ip" => "172.21.1.59"
  "local_port" => 49162
  "http_version" => 3
  "protocol" => 1
  "ssl_verifyresult" => 0
  "scheme" => "HTTP"
  "appconnect_time_us" => 0
  "connect_time_us" => 387726
  "namelookup_time_us" => 24732
  "pretransfer_time_us" => 387954
  "redirect_time_us" => 0
  "starttransfer_time_us" => 695197
  "total_time_us" => 696832
  "debug" => "Due to a bug in curl 7.64.0, the debug log is disabled; use another version to work around the issue."
]
^ array:44 [
  "response_headers" => array:14 [
    0 => "HTTP/1.1 101 Switching Protocols"
    1 => "Connection: Upgrade"
    2 => "Upgrade: h2c"
    3 => "HTTP/2 404 "
    4 => "date: Sat, 11 Apr 2020 08:42:13 GMT"
    5 => "content-type: text/plain; charset=utf-8"
    6 => "content-length: 9"
    7 => "x-backend-header-rtt: 0.002436"
    8 => "server: nghttpx"
    9 => "via: 2 nghttpx"
    10 => "alt-svc: h3-23=":4433"; ma=3600"
    11 => "x-frame-options: SAMEORIGIN"
    12 => "x-xss-protection: 1; mode=block"
    13 => "x-content-type-options: nosniff"
  ]
  "http_code" => 404
  "error" => null
  "canceled" => false
  "http_method" => "GET"
  "user_data" => null
  "start_time" => 1586594532.884
  "redirect_url" => null
  "url" => "http://nghttp2.org/humans.txt"
  "content_type" => "text/plain; charset=utf-8"
  "header_size" => 376
  "request_size" => 213
  "filetime" => -1
  "ssl_verify_result" => 0
  "redirect_count" => 0
  "total_time" => 0.697213
  "namelookup_time" => 0.023356
  "connect_time" => 0.387891
  "pretransfer_time" => 0.388002
  "size_upload" => 0.0
  "size_download" => 9.0
  "speed_download" => 12.0
  "speed_upload" => 0.0
  "download_content_length" => 9.0
  "upload_content_length" => -1.0
  "starttransfer_time" => 0.696877
  "redirect_time" => 0.0
  "primary_ip" => "139.162.123.134"
  "certinfo" => []
  "primary_port" => 80
  "local_ip" => "172.21.1.59"
  "local_port" => 49160
  "http_version" => 3
  "protocol" => 1
  "ssl_verifyresult" => 0
  "scheme" => "HTTP"
  "appconnect_time_us" => 0
  "connect_time_us" => 387891
  "namelookup_time_us" => 23356
  "pretransfer_time_us" => 388002
  "redirect_time_us" => 0
  "starttransfer_time_us" => 696877
  "total_time_us" => 697213
  "debug" => "Due to a bug in curl 7.64.0, the debug log is disabled; use another version to work around the issue."
]

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.

This is for the curl cli
image

the steam index is the same for all frames.

But for php it seems like this

image

the stream index is different so it creates 2 different connections for the requests.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 11, 2020

OK, thanks, fix is in #36422
Actually, this works, but the log is blurred by multi-connections support.
You'll notice by creating the client with new CurlHttpClient(['http_version' => 2], 1), the 1 asking to open only one connection per host.

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?

@1ed
Copy link
Contributor Author

1ed commented Apr 11, 2020

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 CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE via !2?

@nicolas-grekas
Copy link
Member

I think it's not worth it until proven otherwise yes.

@1ed
Copy link
Contributor Author

1ed commented Apr 11, 2020

Ok, fair enough. Thank you very much for your help and the patch!

fabpot added a commit that referenced this issue Apr 12, 2020
…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
@fabpot fabpot closed this as completed Apr 12, 2020
@kelunik
Copy link
Contributor

kelunik commented May 23, 2020

@1ed Can you still reproduce the error you got with AmpHttpClient or was this with an earlier version? Currently I get Amp\Http\Client\HttpException : CONNECT or upgrade request made without upgrade handler callback, but with amphp/http-client directly.

@1ed
Copy link
Contributor Author

1ed commented May 23, 2020

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?

@kelunik
Copy link
Contributor

kelunik commented May 23, 2020

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants