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

Support HTTP/2 ✨ #30

Closed
hawkowl opened this issue Jul 6, 2016 · 16 comments
Closed

Support HTTP/2 ✨ #30

hawkowl opened this issue Jul 6, 2016 · 16 comments

Comments

@hawkowl
Copy link

@hawkowl hawkowl commented Jul 6, 2016

Twisted 16.3.0 landed today, with HTTP/2 support (thanks @Lukasa!). This requires:

a) #10, so that it can negotiate it over TLS
b) Daphne's Site adding h2 as a supported protocol.

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Jul 6, 2016

Actually, it requires more than that. Note that Daphne does not use Site directly, but uses its own HTTPFactory. This means that it needs to, as well, consider overriding and implementing Twisted's new _GenericHTTPProtocol and _GenericHTTPProtocolFactory to use from here: otherwise, the HTTP/2 support will not land.

@hawkowl
Copy link
Author

@hawkowl hawkowl commented Jul 6, 2016

Oh, ugh. Totally didn't look at that... okay, it'll require a little bit more!

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Jul 6, 2016

And to be clear, point b) requires that the HTTPFactory here implement IProtocolNegotiationFactory.

@andrewgodwin
Copy link
Member

@andrewgodwin andrewgodwin commented Jul 6, 2016

Yes, Daphne intercepts the HTTP stack in Twisted at a slightly low level as that seemed to be the only way to get access to all the unprocessed data it needs. Hopefully the existing protocol/factory classes can be extended to match the new interfaces; otherwise, I'm up for moving things around,

@JohnDoee
Copy link
Contributor

@JohnDoee JohnDoee commented Sep 21, 2016

@andrewgodwin Can you elaborate a bit on why it needs to be so low-level? Autobahn websocket runs fine as a Twisted Resource and I can't seem to see anything in http_protocol.py that'd be inaccessible or impossible to make as a Resource.

@andrewgodwin
Copy link
Member

@andrewgodwin andrewgodwin commented Sep 22, 2016

@JohnDoee I don't know, it might be entirely possible to write as a Resource - the autobahn code I was working from at the time didn't use them for the handoff and so I stuck with what was given to me.

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Oct 21, 2016

I should note that, as I look into this a bit further, the fact that Daphne currently does not (and indeed can not) terminate TLS totally removes the ability of Daphne to use HTTP/2. Twisted's HTTP/2 support is HTTPS only.

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Oct 21, 2016

Of course, there's really no good reason for Daphne not to support HTTPS: it just doesn't.

@andrewgodwin
Copy link
Member

@andrewgodwin andrewgodwin commented Oct 21, 2016

Oh I agree. It sounds like if we can make it into a Resource then this all gets a lot easier, I just am focusing on a few other things right now to get Channels 1.0 out the door.

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Nov 18, 2016

Looking at the code, I'm not convinced you need to worry about a Resource yet. Let me dive in a bit further.

@JohnDoee
Copy link
Contributor

@JohnDoee JohnDoee commented Nov 18, 2016

@Lukasa I hope it's a path Daphne will take sooner rather than later as it makes it easier to combine with other Twisted stuff just in general. Other benefits are, of course, smaller code-base, not re-implementing existing stuff.

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Nov 18, 2016

So regardless of the move to Resource, there is a fairly minimal patch that can be applied to Daphne to get HTTP/2 working, as the curl log below shows:

cory@heimdall:daphne/ % better_curl -vk https://178.62.109.34:8001
* Rebuilt URL to: https://178.62.109.34:8001/
*   Trying 178.62.109.34...
* TCP_NODELAY set
* Connected to 178.62.109.34 (178.62.109.34) port 8001 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* successfully set certificate verify locations:
*   CAfile: /usr/local/etc/openssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS header, Certificate Status (22):
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES256-GCM-SHA384
* ALPN, server accepted to use h2
* Server certificate:
*  subject: C=GB; ST=London; L=London; O=Internet Widgits Pty Ltd; CN=localhost
*  start date: Nov 18 13:16:14 2016 GMT
*  expire date: Nov 18 13:16:14 2017 GMT
*  issuer: C=GB; ST=London; L=London; O=Internet Widgits Pty Ltd; CN=localhost
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* 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=0
* Using Stream ID: 1 (easy handle 0x7f9f04800000)
> GET / HTTP/1.1
> Host: 178.62.109.34:8001
> User-Agent: curl/7.51.0
> Accept: */*
> 
* Connection state changed (MAX_CONCURRENT_STREAMS updated)!
< HTTP/2 200 
< content-type: text/html; charset=utf-8
< 
* Curl_http_done: called premature == 0
* Connection #0 to host 178.62.109.34 left intact
Hello world! You asked for /

Unfortunately this patch also requires a small patch in twisted.web's HTTP/2 support, but that should be easily done.

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Nov 18, 2016

See #61.

@andrewgodwin
Copy link
Member

@andrewgodwin andrewgodwin commented Mar 22, 2017

This is now implemented.

@adamchainz
Copy link
Member

@adamchainz adamchainz commented May 29, 2020

@bernardodesousa you should make a new issue for such questions, rather than comment on years-old ones.

@bernardodesousa
Copy link

@bernardodesousa bernardodesousa commented May 29, 2020

Absolutely. I apologize.

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

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.