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

Supports HTTP/2 over clear TCP #722

Closed
wants to merge 1 commit into from

Conversation

diegobes
Copy link

Supports HTTP/2 over clear TCP
— Optimize switching to HTTP/2 by removing calls to init and setup
before switching. Switching will eventually call setup and setup calls
init.

— Supports new version to “force” the use of HTTP/2 over clean TCP

— Add common line parameter “—http2-no-upgrade” to the Curl command line tool.

@bagder bagder added the HTTP/2 label Mar 18, 2016
@@ -150,6 +150,9 @@ version. (Added in 7.33.0)
.IP "--http2"
(HTTP) Tells curl to issue its requests using HTTP 2. This requires that the
underlying libcurl was built to support it. (Added in 7.33.0)
.IP "--http2Clean"
(HTTP) Tells curl to issue its requests using HTTP 2 over clean TCP without HTTP/1.1 Upgrade. This requires that the
underlying libcurl was built to support it. (Added in 7.33.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 7.33 to target. I don't think this will be added until 7.49. 7.48 is a week away. Also wrap at 79

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change command line option to --http2-no-upgrade

@jay
Copy link
Member

jay commented Mar 18, 2016

I think the name clean could be confusing. If http2clean then what is http2 someone might wonder. The "clean" version may sound better.

@jay
Copy link
Member

jay commented Mar 18, 2016

--http2 --no-upgrade maybe?

@bagder
Copy link
Member

bagder commented Mar 25, 2016

Please squash the commits so that we can easier can review/discuss the changes as done to the master branch and not as much as changes within the commit series.

.IP "--http2-no-upgrade"
(HTTP) Tells curl to issue its requests using HTTP 2 over clean TCP without
HTTP/1.1 Upgrade. This requires that the underlying libcurl was built to
support it. (Added in 7.49.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens if this option is set and a HTTPS URL is given? I think the docs should say...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see 2 options:
-- We can ignore this option if there is any output from ALPN ( current implementation )
-- Ignore this option on HTTPS ( not reflected by the current code )

Other idea is not to implement this option as a VERSION but as a new curl-option for the request.

@bagder
Copy link
Member

bagder commented Mar 28, 2016

HTTP_VERSION_2_PRIOR_KNOWLEDGE ?

And I suggest it should enable HTTP/2 for HTTPS connections too.

@diegobes diegobes force-pushed the http/2-over-clean-tcp branch 4 times, most recently from 45a65a4 to 11c36de Compare March 30, 2016 21:40
Supports HTTP/2 over clear TCP
— Optimize switching to HTTP/2 by removing calls to init and setup
before switching. Switching will eventually call setup and setup calls
init.

— Supports new version to “force” the use of HTTP/2 over clean TCP

— Add common line parameter “--http2-prior-knowledge” to the Curl command line tool.
@diegobes
Copy link
Author

The current implementation will enable HTTP/2 over cleartext TCP without the extra round trip of HTTP/1.1 Upgrade. It will also enable HTTP/2 over TLS if no next protocol negotiation available.
To test with curl the new --http2-prior-knowledge parameter can be used

@bagder
Copy link
Member

bagder commented Mar 31, 2016

Lovely. I still lack a bit in the docs, but I'll add some extra text there.

Have you considered if we can add some code to better detect if the server actually doesn't support HTTP/2 even though prior-knowledge is used? If I try it on my http1 server I get this:

$ curl --http2-prior-knowledge localhost
curl: (56) Unexpected EOF

I just think it would be neat if we could detect it and say something better than "Unexpected EOF" for this case!

@bagder
Copy link
Member

bagder commented Mar 31, 2016

Merged! Your commit is 324a97e, my docs follow-up is 213d3c7.

@diegobes
Copy link
Author

About

$ curl --http2-prior-knowledge localhost
curl: (56) Unexpected EOF

I agree, (56) is not great. I will think about it a little more.

Thanks

@jay
Copy link
Member

jay commented Apr 1, 2016

If the purpose of this is forcing http2 why don't we just call it --http2-force instead of --http2-prior-knowledge

@bagder
Copy link
Member

bagder commented Apr 1, 2016

I agree that --http2-prior-knowledge is an uncomfortably long name and I wouldn't mind a shorter. But the term "prior knowledge" comes from the HTTP/2 spec and refers to the fact that a user (of curl) must have prior knowledge that the server is actually supporting HTTP/2 over plain TCP for this to work.

I wouldn't say that --http2-force necessarily would mean that to a user. But maybe?

@bagder
Copy link
Member

bagder commented Apr 1, 2016

@tatsuhiro-t, when we try to speak HTTP/2 with a server that is responding with HTTP/1, nghttp2_session_mem_recv() still returns a valid number that looks like it thought it was OK. Is that really intended?

I tried curl --http2-prior-knowledge http://google.se -v and the server returns "400 Bad Request\r\nContent-Type: text/html; charset=UTF-8\r\nCont... (and some more) which I would suspect nghttp2 quite immediately could detect as being wrong.

I also tried to set an error callback, but nothing is told to that one either. Am I thinking wrong?

I'm looking for ways to bring back a nicer message to curl users that they tried http2 prior knowledge to a server that clearly is not speaking HTTP/2...

@tatsuhiro-t
Copy link
Contributor

nghttp2 consumes input byte as much as possible unless there is fatal error (e.g., out of memory). Even if nghttp2_mem_recv() returns the number of all input bytes, it does not mean all bytes are correct and valid. nghttp2 might set GOAWAY to terminate connection.
If server responds in HTTP/1 fashion, nghttp2 reads it as HTTP/2 frame, and it will see that it is not SETTINGS frame, and with some long length, and will issue GOAWAY.

The error callback function (nghttp2_error_callback) introduced in the 1.9.0, is quite limited as of now. It only reports the error about incoming HTTP header fields. We can add about this case to invoke error callback in the future release.

@bagder
Copy link
Member

bagder commented Apr 1, 2016

Thanks @tatsuhiro-t, I figured that was the situation. Do you have any recommendation on how we best detect this situation. I mean when we try to talk to HTTP/2 to a server that clearly does not understand it? The point would be to attempt to give back a better explanation to the user of why it doesn't work, nghttp2 otherwise handles the situation just fine and closes down nicely.

@tatsuhiro-t
Copy link
Contributor

We added error callback call when nghttp2 did not receive unexpected frame header while it expected initial SETTINGS frame. nghttp2/nghttp2@5974aba
curl can print out this error message to the user to indicate that prior knowledge is wrong.
Do you think this fixes the issue? Or do we need more elaborated work to handle this case?

@bagder
Copy link
Member

bagder commented Apr 2, 2016

Nice, yes that helps a lot, thanks. Now I just need to make use of the error_callback properly - I've tested your new message in a test branch successfully.

bagder added a commit that referenced this pull request Apr 2, 2016
It offers extra info from nghttp2 in certain error cases. Like for
example when trying prior-knowledge http2 on a server that doesn't speak
http2 at all. The error message is passed on as a verbose message to
libcurl.

Discussed in #722

The error callback was added in nghttp2 1.9.0
@bagder
Copy link
Member

bagder commented Apr 2, 2016

With that, I think we can conclude this pull request to be "done" for now. Thanks all for your help.

@bagder bagder closed this Apr 2, 2016
@diegobes
Copy link
Author

diegobes commented Apr 5, 2016

Thanks !!!
I just saw the messages thread ( vacation :) )

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants