Add force_proto kwarg on contrib HTTP20Adapter

[yuvadm]

This is the version I'm using for my project.

If the API kwargs I added don't make sense upstream, I'll be happy to make any changes to make it applicable.

[Lukasa]

Rather than grab this out of kwargs, let's make it an explicit keyword argument.

[yuvadm]

You got it. Would you rather I edit and squash the commit, or just add a new one?

[Lukasa]

New commits are fine. =) I'm not that precious about history.

[Lukasa]

I think this is only partially complete. The requests adapter uses the generic HTTP connection object, which will by default try to use the HTTP/1.1 connection object and upgrade to HTTP/2. However, the HTTP/1.1 connection object won't see force_proto and so will never react to the forced protocol change.

I think you need to plumb this through into the HTTP/1.1 connection object, much like on the HTTP/2 connection object, to get this to behave as expected. It also won't work for plaintext HTTP/2, which might mean that we should do some magic in the generic connection object to ensure that it just auto-selects the right thing.

[yuvadm]

@Lukasa I added the same params in HTTP11Connection. However, I'm not familiar with the magic required for plaintext HTTP/2. Pointers?

[Lukasa]

@yuvadm Well, we could do it "properly", but I think that's actually inadvisable: if we do it properly we'll make a HTTP/1.1 request with the upgrade header but then ignore the response from the server and forcibly upgrade it. I think that's a bad idea.

Instead, let's check the force_proto kwarg in the __init__ of the generic connection object. If it's h2, we should not create a HTTP/1.1 connection but a HTTP/2 connection. Otherwise, we should do what we do right now.

[Lukasa]

Bump. =)

[yuvadm]

@Lukasa I got a bit hung up with this, sorry :) This is currently the diff I have on my working copy. Is this the right direction?

diff --git a/hyper/common/connection.py b/hyper/common/connection.py
index c867113..4916e69 100644
--- a/hyper/common/connection.py
+++ b/hyper/common/connection.py
@@ -62,7 +62,7 @@ class HTTPConnection(object):
         self._host = host
         self._port = port
         self._h1_kwargs = {
-            'secure': secure, 'ssl_context': ssl_context, 'force_proto': force_proto,
+            'secure': secure, 'ssl_context': ssl_context,
             'proxy_host': proxy_host, 'proxy_port': proxy_port
         }
         self._h2_kwargs = {
@@ -75,9 +75,17 @@ class HTTPConnection(object):
         self._h1_kwargs.update(kwargs)
         self._h2_kwargs.update(kwargs)

-        self._conn = HTTP11Connection(
-            self._host, self._port, **self._h1_kwargs
-        )
+        if 'force_proto':
+            # When using `force_proto` we skip the HTTP/1.1 connection
+            # since we'll be ignoring the server response anyway
+            # so just go straight to an HTTP/2 connection
+            self._conn = HTTP20Connection(
+                self._host, self._port, **self._h2_kwargs
+            )
+        else:
+            self._conn = HTTP11Connection(
+                self._host, self._port, **self._h1_kwargs
+            )

     def request(self, method, url, body=None, headers={}):
         """

[Lukasa]

No need to apologise! We're all busy: just wanted to make sure this doesn't languish, especially as I'm hoping to do a release of hyper sometime in the near future.

Yeah, that's very much the direction I'm expecting to see us go. The patch isn't quite right: we want to check if force_proto is h2 before we switch. =)