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
base: master
from

Conversation

Projects
None yet
4 participants
@diegobes

diegobes commented Mar 18, 2016

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

Show outdated Hide outdated docs/curl.1
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 18, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 18, 2016

Member

--http2 --no-upgrade maybe?

Member

jay commented Mar 18, 2016

--http2 --no-upgrade maybe?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 25, 2016

Member

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.

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.

@diegobes diegobes closed this Mar 26, 2016

@diegobes diegobes reopened this Mar 26, 2016

Show outdated Hide outdated docs/curl.1
Show outdated Hide outdated include/curl/curl.h
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 28, 2016

Member

HTTP_VERSION_2_PRIOR_KNOWLEDGE ?

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

Member

bagder commented Mar 28, 2016

HTTP_VERSION_2_PRIOR_KNOWLEDGE ?

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

Diego Bes Diego Bes
Supports HTTP/2 over clear TCP
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

This comment has been minimized.

Show comment
Hide comment
@diegobes

diegobes Mar 31, 2016

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

diegobes commented Mar 31, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 31, 2016

Member

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!

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 31, 2016

Member

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

Member

bagder commented Mar 31, 2016

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

@diegobes

This comment has been minimized.

Show comment
Hide comment
@diegobes

diegobes Mar 31, 2016

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

diegobes commented Mar 31, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Apr 1, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 1, 2016

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 1, 2016

Member

@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...

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

This comment has been minimized.

Show comment
Hide comment
@tatsuhiro-t

tatsuhiro-t Apr 1, 2016

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.

Contributor

tatsuhiro-t commented Apr 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 1, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@tatsuhiro-t

tatsuhiro-t Apr 2, 2016

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?

Contributor

tatsuhiro-t commented Apr 2, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 2, 2016

Member

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.

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

http2: make use of the nghttp2 error callback
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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 2, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@diegobes

diegobes Apr 5, 2016

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

diegobes commented Apr 5, 2016

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

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