Plaintext http20 via http11 upgrade

[fredthomsen]

Still needs some cleanup and test cases need to be added and updated.

[Lukasa]

Fantastic @fredthomsen! I'm pretty wiped out this evening so I'll take a look tomorrow and give you some more detailed feedback.

[Lukasa]

So let's start with the good news: this totally works, at least in the most simple case! I've been quite successful in getting http2bin.org to happily take the plaintext H2 upgrade, which is really awesome.

I'll start providing some code review here, but this is looking really good. @fredthomsen, if you find at any point that you don't think you want to spend the time on test cases, let me know and I'm happy to take over and clean this up and write tests.

[Lukasa]

The bytes() call here doesn't work on Python 2, so it might be better to replace it with a .decode('utf-8') call.

[Lukasa]

Ok, basic code review is done.

This is totally amazing, I'm so excited to add this. I definitely owe you one @fredthomsen! 🍰

[Lukasa]

Can you add a space to the left of this parenthesis?

[fredthomsen]

Thanks for the feedback. I'll make these changes today. One concern I had:
-The first stream id will never be returned for the initial request since we need to parse the response to find out if we were actually upgraded. This means that this behavior/return value differs for the request function for the HTTPConnection proxy object for TLS upgrades vs HTTP upgrades.

I seem to be having some issues with my tox and/or py.test setup as my test execution is hanging. Once I get the squared up then I will address any test failures and add additional ones as needed.

[Lukasa]

The first stream id will never be returned for the initial request since we need to parse the response to find out if we were actually upgraded. This means that this behavior/return value differs for the request function for the HTTPConnection proxy object for TLS upgrades vs HTTP upgrades.

Yeah, that's annoying. Right now the API has a limitation there, in that there are no stream IDs returned if you're using HTTP/1.1. For the moment that might be OK.

Longer term, we may be able to extend the HTTP/1.1 API to nominally have stream IDs, but for that to always be stream ID 1, for example. Or possibly None.

[fredthomsen]

Was looking into some of the test case failures and wondering how to handle test_hitting_http2bin_org_http11 in test_release. This test case depended on non-secure traffic on an HTTP11Connection not getting upgraded. Should there be a flag or something completely disabling the upgrade mechanism?

[Lukasa]

The easiest thing to do is to simply change the host we hit. httpbin.org should work fine.

[fredthomsen]

Ok. All the changes discussed above have been made and test cases updated to support this. Any other test cases you believe are needed for this feature?

[Lukasa]

Hmm, the tests all blew up. I wonder if that's a timing problem.

[Lukasa]

Those failures don't appear to be timing related, because they occur on my local machine as well.

[fredthomsen]

Only the integration tests have an issue on my box, the rest are passing. I'll take a look at the CI logs and try to straighten it out.

[Lukasa]

Alrighty, I've worked it out. I'll do this merge manually and hopefully that'll fix up the tests.

[Lukasa]

Merged! 🍰 ✨ 🍰 Thanks so much @fredthomsen! You'll find your name in CONTRIBUTORS.rst: feel free to keep contributing code, we'd love to have it!

[fredthomsen]

Thanks for the help and feedback. Were all those test case failures resolved with the change made to the SocketLevelTest in server.py?

[Lukasa]

The test failures were indeed resolved, however we're short some test coverage, as mentioned in #137. If you feel like adding it let me know, otherwise I'll add those tests tomorrow.