add HTTP version to connection and responses #296

nateprewitt · 2016-12-01T18:24:31Z

This should address #285.

The tests aren't passing right now because we still have an outstanding bug (#291) in hyper until a new version of h2 is released. Other than the b'connection' header failures though, everything seems to be working.

nateprewitt · 2016-12-01T18:35:33Z

Actually it looks like TestImportPython2.test_cannot_import_python_2 is failing too because it raises an AttributeError rather than an ImportError.

This seems unrelated to this patch though. I can dig a bit if you don't immediately know what's causing that @Lukasa.

Lukasa

This generally seems fine! I'll try to chase the test stuff up tomorrow.

Lukasa · 2016-12-01T21:59:52Z

hyper/http20/connection.py

@@ -102,6 +104,9 @@ def __init__(self, host, port=None, secure=None, window_manager=None,
        else:
            self.host, self.port = host, port

+        # HTTP Version of the connection
+        self.version = HTTPVersion.http20


Given that these are static and don't change, let's set these on the class object rather than do this at initialisation time.

Lukasa · 2016-12-01T22:01:05Z

test/test_hyper.py

+    def test_response_version(self):
+        r = HTTP20Response(HTTPHeaderMap([(':status', '200')]), None)
+        assert r.version is HTTPVersion.http20
+


Can we also get a few tests for the abstract HTTPConnection object passing this through?

nateprewitt · 2016-12-02T05:51:21Z

A couple quick questions on the abstract tests. I'm currently unable to come up with a way to test HTTP and TLS upgrades without live network IO to something like http2bin.org. I could mock the connections like the other tests but that kind of defeats the point.

The second question I have is more of an oddity. I've written what is a successful test when run with py.test and as a separate python file, but in tox it repeatedly fails to perform the TLS upgrade. wrap_socket never changes the proto from None.

from hyper.common.connection import HTTPConnection
from hyper.common.util import HTTPVersion

c = HTTPConnection('www.http2bin.org', 443)
assert c.version is HTTPVersion.http11
c.request('GET', '/')
assert c.version is HTTPVersion.http20

nateprewitt · 2016-12-02T05:53:45Z

test/test_abstraction.py

+        c = HTTPConnection('www.http2bin.org', 80)
+        assert c.version is HTTPVersion.http11
+        c.request('GET', '/')
+        c.get_response()


Note: test the response version here too.

Lukasa · 2016-12-02T10:30:20Z

I'm currently unable to come up with a way to test HTTP and TLS upgrades without live network IO to something like http2bin.org. I could mock the connections like the other tests but that kind of defeats the point.

I don't think I follow this. Other parts of the test suite have found a way to do it, it should be possible for your code to do that. In this case, you just need to lie about the connection setup, which ought not to be too difficult.

I've written what is a successful test when run with py.test and as a separate python file, but in tox it repeatedly fails to perform the TLS upgrade. wrap_socket never changes the proto from None.

Are you running the same Python version in both cases? Same dependencies? Almost certainly you're using a Python that has a too-old ssl library or OpenSSL in tox, so there is no negotiation.

Lukasa · 2016-12-02T11:49:03Z

Ok, the master branch should have its tests fixed up.

nateprewitt · 2016-12-02T15:19:05Z

I don't think I follow this. Other parts of the test suite have found a way to do it, it should be possible for your code to do that. In this case, you just need to lie about the connection setup, which ought not to be too difficult.

Sorry, I should have been clearer. The other parts of the library are mocking out a custom Connection object in order to do this. In that case, I'm simply creating a DummySocket that has an attr "version" and then testing the dummy socket still has a version after upgrades. That kind of test never actually touches the underlying HTTP11Connection or HTTP20Connection so it doesn't prove anything useful. It will always pass regardless of changes to the actual connection classes.

Are you running the same Python version in both cases? Same dependencies? Almost certainly you're using a Python that has a too-old ssl library or OpenSSL in tox, so there is no negotiation.

That was the first thing I had checked but couldn't find anything immediately. The weird thing is it will run fine locally in a fresh virtualenv, but fails in tox and Travis. I'll try to do a deeper dig to figure out what my base environment has that doesn't seem to be in Travis.

Lukasa · 2016-12-02T15:20:05Z

Hrm. Can you do it in the socket-level style by actually creating a TLS connection? If not, we may need to use some mocks to achieve what we need.

nateprewitt · 2016-12-02T15:44:47Z

Yeah, I'll do some tinkering this afternoon and try to devise a way to initiate the upgrades with the actual objects but no external IO.

Lukasa

So this looks good, except for the fact that one of the tests is super-wacky and I don't really understand what it was trying to do.

Lukasa · 2016-12-06T10:16:53Z

test/test_abstraction.py

+                b'Server: socket-level-server\r\n'
+                b'Content-Length: 0\r\n'
+                b'Connection: upgrade\r\n',
+                b'Upgrade: TLS/1.0, HTTP/2.0'


Uh...what is this? This test doesn't make any sense at all, and frankly suggests that the upgrade code in hyper is broken. =P

Sorry, this one was a test based off of a bad reading of the RFC 2817. I had a question on it yesterday that I was going to ask here but solved it and forgot to remove the code.

nateprewitt · 2016-12-06T23:48:37Z

@Lukasa, okay, sorry that took so long. I got the test working by monkeypatching wrap_socket. Things should be ready for an actual look now.

nateprewitt · 2016-12-07T00:39:25Z

As an aside, is there a reason the tests are configured to pass despite failures, as long as there's 100% test coverage?

I hit this with a couple test attempts that were failing but the build wasn't explicitly telling me until I dug into the logs of "successful" builds. It looks like all of the NGHTTP2 builds have been failing to import nghttp2 (TestUtilities.test_nghttp2_installs_correctly) for at least the last few months too.

Lukasa · 2016-12-07T08:24:01Z

There is not, I think it's just a misconfiguration of Travis. I seem to recall that I had the same problem in the hpack library, which I fixed in python-hyper/hpack#60. This included removing the nghttp2 support (which hasn't worked for a while, and which I didn't have the resources to investigate).

I recommend opening a PR to do both of those things if you have the bandwidth to do it. =)

Lukasa

Small notes, but I think it'd be good if we tackled the bad reporting of tests first. =)

Lukasa · 2016-12-07T08:29:19Z

test/test_integration.py

+            sock.sendall(f.serialize())
+
+            # Wait for the message from the main thread.
+            send_event.set()


This needs to be wait, not set.

Lukasa · 2016-12-07T10:21:43Z

Nevermind, I'm tackling it myself in #298.

nateprewitt · 2016-12-07T16:23:50Z

Alright, things should be set once the tests finish.

Lukasa

Looks good to me, thanks @nateprewitt!

Lukasa suggested changes Dec 1, 2016

View reviewed changes

nateprewitt force-pushed the 285 branch from 2d3ce9c to b9e6dea Compare December 2, 2016 04:38

nateprewitt commented Dec 2, 2016

View reviewed changes

adding HTTP version flags to connections and responses

e76f185

nateprewitt force-pushed the 285 branch from b9e6dea to e76f185 Compare December 2, 2016 15:45

Lukasa suggested changes Dec 6, 2016

View reviewed changes

nateprewitt force-pushed the 285 branch 2 times, most recently from 8f3b8c5 to b7a5758 Compare December 6, 2016 23:14

nateprewitt force-pushed the 285 branch from b7a5758 to 772d960 Compare December 7, 2016 00:23

Lukasa suggested changes Dec 7, 2016

View reviewed changes

adding itegration tests for HTTPConnection

bdcf38e

nateprewitt force-pushed the 285 branch from 772d960 to bdcf38e Compare December 7, 2016 16:19

Lukasa approved these changes Dec 7, 2016

View reviewed changes

Lukasa merged commit 80ecee4 into python-hyper:development Dec 7, 2016

Lukasa mentioned this pull request Dec 7, 2016

Add a flag to connection and to response objects to find out connection type. #285

Closed

nateprewitt deleted the 285 branch December 12, 2016 21:42

nateprewitt mentioned this pull request Jun 14, 2017

Response knows whether the request went over HTTP/1.x or HTTP/2 #330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add HTTP version to connection and responses #296

add HTTP version to connection and responses #296

nateprewitt commented Dec 1, 2016

nateprewitt commented Dec 1, 2016

Lukasa left a comment

Lukasa Dec 1, 2016

Lukasa Dec 1, 2016

nateprewitt commented Dec 2, 2016

nateprewitt Dec 2, 2016

Lukasa commented Dec 2, 2016

Lukasa commented Dec 2, 2016

nateprewitt commented Dec 2, 2016

Lukasa commented Dec 2, 2016

nateprewitt commented Dec 2, 2016

Lukasa left a comment

Lukasa Dec 6, 2016

nateprewitt Dec 6, 2016 •

edited

nateprewitt commented Dec 6, 2016

nateprewitt commented Dec 7, 2016

Lukasa commented Dec 7, 2016

Lukasa left a comment

Lukasa Dec 7, 2016

Lukasa commented Dec 7, 2016

nateprewitt commented Dec 7, 2016

Lukasa left a comment

add HTTP version to connection and responses #296

add HTTP version to connection and responses #296

Conversation

nateprewitt commented Dec 1, 2016

nateprewitt commented Dec 1, 2016

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa Dec 1, 2016

Choose a reason for hiding this comment

Lukasa Dec 1, 2016

Choose a reason for hiding this comment

nateprewitt commented Dec 2, 2016

nateprewitt Dec 2, 2016

Choose a reason for hiding this comment

Lukasa commented Dec 2, 2016

Lukasa commented Dec 2, 2016

nateprewitt commented Dec 2, 2016

Lukasa commented Dec 2, 2016

nateprewitt commented Dec 2, 2016

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa Dec 6, 2016

Choose a reason for hiding this comment

nateprewitt Dec 6, 2016 • edited

Choose a reason for hiding this comment

nateprewitt commented Dec 6, 2016

nateprewitt commented Dec 7, 2016

Lukasa commented Dec 7, 2016

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa Dec 7, 2016

Choose a reason for hiding this comment

Lukasa commented Dec 7, 2016

nateprewitt commented Dec 7, 2016

Lukasa left a comment

Choose a reason for hiding this comment

nateprewitt Dec 6, 2016 •

edited