feat(client): initial support for HTTP/2 #448

mlalic · 2015-04-11T17:17:24Z

This PR adds initial support for HTTP/2 requests.

It introduces a new pair of traits: HttpMessage and Protocol.

The HttpMessage trait provides an API that abstracts the sending and receiving of HTTP messages away from the concrete implementation of the underlying protocol.

The Protocol trait provides an API for constructing new HttpMessages.

The Request and Response structs are refactored to internally use a Box<HttpMessage> for their HTTP communication requirements.

The Client has a new public method: with_protocol. This provides an API for plugging in a particular Protocol implementation that will then be used for constructing the HttpMessage instances that are passed to the Request struct constructor.

The implementation of this API (i.e. the two traits) for HTTP/2 is provided in the hyper::http2 module.

Therefore, now it is possible to inject an Http2Protocol instance when constructing a hyper::Client which seamlessly makes it switch to using HTTP/2.

The Http2Protocol can use the existing NetworkConnector API for constructing the network connection for the underlying communication. However, do note that the current TLS connector does not honor the requirements that the HTTP/2 spec additionally makes for TLS connections (particular cipher suits are banned, applications must use ALPN -- or at least NPN -- for protocol negotiation), so if such a connector is provided, it won't be possible to establish an HTTP/2 connection. The cleartext TCP connectors work as expected.

An example is included in examples/client_http2.rs showing how the client can be configured to use HTTP/2.

mlalic@skaro:~/src/hyper$ cargo run --example client_http2 -- http://http2bin.org/get?hi=hi
     Running `target/debug/examples/client_http2 http://http2bin.org/get?hi=hi`
Response: 200 OK
Headers:
x-clacks-overhead: GNU Terry Pratchett
access-control-allow-origin: *
:status: 200
content-type: application/json
server: h2o/1.1.1
date: Fri, 29 May 2015 12:29:14 GMT
access-control-allow-credentials: true

{
  "args": {
    "hi": "hi"
  }, 
  "headers": {
    "Connection": "keep-alive", 
    "Host": "http2bin.org,http2bin.org", 
    "Via": "2 http2bin.org"
  }, 
  "origin": "131.159.207.123", 
  "url": "http://http2bin.org,http2bin.org/get?hi=hi"
}

Breaking Changes

The PR introduces only a few minor, however still breaking changes:

The hyper::client::Response struct no longer requires any type parameters.
The hyper::Error public enum has gained a new variant Http2

seanmonstar · 2015-04-11T20:47:27Z

So excite.

seanmonstar · 2015-04-11T23:23:46Z

Cargo.toml

@@ -25,6 +25,8 @@ url = "*"
 traitobject = "*"
 typeable = "*"

+[dependencies.solicit]
+git = "https://github.com/mlalic/solicit"


Not on crates.io yet?

Yeah, not yet... I wanted to maintain some flexibility, without pushing multiple versions out to crates.io, if it turned out that I needed to do some changes there to make things work better in hyper (so far it turned out that I didn't).

Of course it's something to do before finally landing this to make sure hyper is pinned to a proper version.

bgdncz · 2015-04-13T17:52:38Z

+1, been waiting for this ever since HTTP2 came out :)

reem · 2015-04-13T19:09:20Z

@mlalic might be worth rewriting commits now just to get gitcop to shut up (I just deleted its comments since it's noise right now).

reem · 2015-04-13T19:12:53Z

src/client/mod.rs

+    /// `new()` call. This, however, means that all particular
+    /// `HttpRequestFactory` implementations must have a set of default
+    /// parameters with which they can be created.
+    fn new() -> Self;


We could get rid of this and just have HttpRequestFactory inherit from Default.

GitCop · 2015-04-15T09:29:48Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 3ce492a
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

GitCop · 2015-04-15T09:36:55Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 3ce492a
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

mlalic · 2015-04-15T09:42:09Z

So I've updated the PR based on the discussion so far: rebased the fixes that were made along the way to the corresponding commits and adapted the commit messages to the style guidelines (except the one that needs to be rebased out of the tree anyway!).

The status is that with this, the public API of the client seems to be very nearly unaffected.

The return type is now HttpResult<HttpResponse>, where HttpResponse is a new struct that has the same public API as the previous Response struct, apart from not being generic. Therefore, all the examples and tests can work without any changes, but if anyone relied on the fact that this public type is generic, that code would break. Obviously, the name of the type is also different, but we could easily call this one Response and rename the old one, although in that case a different type of "breakage" occurs, since Response was a publicly exposed type, someone might have used it directly, instead of going through the Client (though very unlikely).

For now I also removed the with_connector method, but we could keep that in and instantiate an HTTP/1.1 request factory in the implementation so as to keep that change also completely transparent.

mlalic · 2015-04-18T19:13:38Z

So, what do you guys (@seanmonstar, @reem) think: should we go down this route? If so, I can bang this into shape with proper test coverage, documentation, error handling, etc.

seanmonstar · 2015-04-21T17:49:37Z

@mlalic sorry, been busy. I'll look it over again today.

seanmonstar · 2015-04-29T20:35:44Z

@mlalic I know Servo uses Request::with_connector, cause the XHR spec defines some fun rules slightly different than standard HTTP. /cc @Manishearth

I have a Pool implementation in #486. That would allow keeping the requests around, and having h1 -> h2 upgrade functionality.

What did you think of my previous suggestion of keeping Client, Request, and Response as opaque structs, and having the trait be a HttpMessage, implemented by http1 and http2? I'd like to think this would ease implementing on the server as well...

mlalic · 2015-04-30T18:04:00Z

@seanmonstar

I know Servo uses Request::with_connector, cause the XHR spec defines some fun rules slightly different than standard HTTP

Indeed, currently the establishment of the underlying TCP connection that the HTTP/2 connection would use is somewhat hidden away, but letting it use the NetworkConnector abstraction for that should be quite simple. The NetworkConnector produces an object that is both Read and Write, which is all that solicit's HTTP/2 connection requires. The custom connectors that Servo may already have should then work regardless of whether the tcp connection is used for h1 or h2.

I have a Pool implementation in #486. That would allow keeping the requests around, and having h1 -> h2 upgrade functionality.

The way the pool is implemented right now doesn't lend itself at all to reusing connections for HTTP/2. What is currently pooled are the tcp connections, not HTTP connections. This is virtually the same for h1, but it is not so for h2. This is because in h1, you can simply send the raw request bytes on one of the established tcp streams and you're good; no previously sent requests on that particular stream matter for the raw payload that you transmit over the wire, nor for the parsing of the response payload. In h2, however, the payload you send for a request does depend on previously sent requests - you need to assign a new stream id, hpack encoding/decoding the headers is stateful, flow control windows depend on previously sent data, etc. Therefore, simply pooling the TCP connections does nothing to allow "keep alive"-like usage of an HTTP/2 connection.

Of course, it's doable to pool the actual HTTP 2 connections, by doing what you've done on the connector level within (what is currently named in this pr) the HttpRequestFactory. In fact, if acquiring a lock before creating a new request is already deemed acceptable, I could even squeeze my async client into the same api, allowing multiple concurrently issued requests to the same host to use the same HTTP 2 connection, as long as the original factory instance is kept alive and its clones given to new hyper clients (provided that spawning an io thread is also acceptable).

What did you think of my previous suggestion of keeping Client, Request, and Response as opaque structs, and having the trait be a HttpMessage, implemented by http1 and http2? I'd like to think this would ease implementing on the server as well...

I don’t think I fully understood what you were aiming at with the HttpMessage trait. From what I can gather, it should be a type that exposes an interface for queuing HTTP "messages" (set_outgoing), as well as for grabbing them off a connection (get_incoming). The Read/Write trait bounds were for reading/writing the body?

The way it looks like it would be used would to implement the request's Fresh -> Streaming -> Response transformation would be to first queue the MessageHead (i.e. Fresh -> Streaming), then stream out the response (calls to the underlying Write?) and grab the response head (i.e. Streaming -> Response), and finally read out the response (calls to the Read). All this has already become encapsulated by the FreshHttpRequest and StreamingHttpRequest traits. The implementations of these two traits, along with a corresponding factory for HTTP/1.1 and HTTP/2 fully encapsulate all relevant protocol logic.

So, I don’t see the benefit of this alternate approach. It seems it would only shift the actual protocol logic a level lower... Is it that you hope to reuse this HttpMessage component to support server-side HTTP/2, as well?

Originally, when you wrote that comment, I thought the suggestion was so that we avoid having type parameters on the Client, which I’ve achieved by making the Response struct fully independent from the protocol that produces it. (The original post is what nudged me into realizing it was easily doable.)

Perhaps you could flesh out your proposal more because I just might not be fully getting the idea from the previous short description.

seanmonstar · 2015-05-05T01:52:14Z

I'll expore in a branch to show what I mean.

mlalic · 2015-05-25T22:56:42Z

@seanmonstar I have an experiment over on this branch (forked off the current master HEAD, not this PR) refactoring the HTTP/1.1-specific parts of Request/Response into an implementation of an HttpMessage trait, such as what you described.

Is this sort of what you had in mind for the HttpMessage API?

seanmonstar · 2015-05-27T06:28:58Z

Pretty much exactly. As long as such a trait could work for http2, of
course.

On Mon, May 25, 2015, 3:56 PM Marko Lalic [email protected] wrote:

@seanmonstar https://github.com/seanmonstar I have an experiment over
on this branch
master...mlalic:http-message
(forked off the current master HEAD, not this PR) refactoring the
HTTP/1.1-specific parts of Request/Response into an implementation of an
HttpMessage trait, such as what you described.

Is this sort of what you had in mind for the HttpMessage API?

—
Reply to this email directly or view it on GitHub
#448 (comment).

coveralls · 2015-05-29T12:30:08Z

Coverage decreased (-2.11%) to 87.44% when pulling 984f39e on mlalic:http2-initial into 1e5d7d4 on hyperium:master.

mlalic · 2015-05-29T12:42:45Z

@seanmonstar

The trait does work out quite nicely for HTTP/2 too.

So, here's an attempt at landing the PR, this time with everything polished up with proper docs and tests for the new features. Feedback of course appreciated.

(Side note: the HttpMessage API would allow for some nicer tests for the Client itself. Right now, it seems very difficult -- or at least extremely kludgy -- to test what the client itself sends, regardless of the protocol. For example, I wanted to write a test which checks whether the client properly handles redirects, as by looking at the code it seems like it always sticks to the original request method, regardless of the redirect status code it sees, but I gave up once I saw that writing a test that checks exactly what requests got sent wasn't as easy as I'd hoped. With the HttpMessage API we can just test which messages got dispatched, making it all much smoother.)

seanmonstar · 2015-06-01T16:58:28Z

Sorry, I was camping this weekend (so no Internet), but this looks exciting!

seanmonstar · 2015-06-01T17:36:08Z

src/error.rs

@@ -108,6 +114,12 @@ impl From<httparse::Error> for Error {
    }
 }

+impl From<Http2Error> for Error {
+    fn from(err: Http2Error) -> Error {
+        Error::Http2(err)


Could you add a from_and_cause! case to the tests below, to keep the coverage happy.

BREAKING CHANGE: `hyper::client::request::Response` is no longer generic over `NetworkStream` types. It no longer requires a generic type parameter at all.

Automatic conversion from the `solicit::http::HttpError` is also provided. BREAKING CHANGE: A new variant `Http2` added to a public enum `hyper::Error`.

coveralls · 2015-06-02T14:09:53Z

Coverage decreased (-1.54%) to 88.78% when pulling 6504936 on mlalic:http2-initial into 0cd7e9d on hyperium:master.

seanmonstar · 2015-06-02T16:55:00Z

Just curious, from the example: "Host": "http2bin.org,http2bin.org",, is it supposed to have the host doubled like that?

feat(client): initial support for HTTP/2

seanmonstar · 2015-06-02T17:12:15Z

@mlalic woo! thanks for this herculean effort!

mlalic · 2015-06-03T06:28:40Z

Happy to do it! Glad we've worked out the initial integration; we can take it from here and tweak/improve things incrementally, of course... Should be less of a big-bang than this :)

mlalic · 2015-06-03T06:32:44Z

Just curious, from the example: "Host": "http2bin.org,http2bin.org",, is it supposed to have the host doubled like that?

hyper probably includes the Host header and the HTTP/2 client includes the :authority pseudo-header so that's probably where it ends up looking doubled.

:authority "should" be used in HTTP/2 requests, but host is still fine, so we can tweak it that the Http2Message doesn't set any of those unless they're given to it in the message head? (Like HTTP/1.1 at least one of those headers is still required for every request, so it means that the Request always needs to set it...)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): initial support for HTTP/2 #448

feat(client): initial support for HTTP/2 #448

mlalic commented Apr 11, 2015

seanmonstar commented Apr 11, 2015

seanmonstar Apr 11, 2015

mlalic Apr 12, 2015

bgdncz commented Apr 13, 2015

reem commented Apr 13, 2015

reem Apr 13, 2015

mlalic Apr 14, 2015

GitCop commented Apr 15, 2015

GitCop commented Apr 15, 2015

mlalic commented Apr 15, 2015

mlalic commented Apr 18, 2015

seanmonstar commented Apr 21, 2015

seanmonstar commented Apr 29, 2015

mlalic commented Apr 30, 2015

seanmonstar commented May 5, 2015

mlalic commented May 25, 2015

seanmonstar commented May 27, 2015

coveralls commented May 29, 2015

mlalic commented May 29, 2015

seanmonstar commented Jun 1, 2015

seanmonstar Jun 1, 2015

mlalic Jun 2, 2015

coveralls commented Jun 2, 2015

seanmonstar commented Jun 2, 2015

seanmonstar commented Jun 2, 2015

mlalic commented Jun 3, 2015

mlalic commented Jun 3, 2015

feat(client): initial support for HTTP/2 #448

feat(client): initial support for HTTP/2 #448

Conversation

mlalic commented Apr 11, 2015

Breaking Changes

seanmonstar commented Apr 11, 2015

seanmonstar Apr 11, 2015

Choose a reason for hiding this comment

mlalic Apr 12, 2015

Choose a reason for hiding this comment

bgdncz commented Apr 13, 2015

reem commented Apr 13, 2015

reem Apr 13, 2015

Choose a reason for hiding this comment

mlalic Apr 14, 2015

Choose a reason for hiding this comment

GitCop commented Apr 15, 2015

GitCop commented Apr 15, 2015

mlalic commented Apr 15, 2015

mlalic commented Apr 18, 2015

seanmonstar commented Apr 21, 2015

seanmonstar commented Apr 29, 2015

mlalic commented Apr 30, 2015

seanmonstar commented May 5, 2015

mlalic commented May 25, 2015

seanmonstar commented May 27, 2015

coveralls commented May 29, 2015

mlalic commented May 29, 2015

seanmonstar commented Jun 1, 2015

seanmonstar Jun 1, 2015

Choose a reason for hiding this comment

mlalic Jun 2, 2015

Choose a reason for hiding this comment

coveralls commented Jun 2, 2015

seanmonstar commented Jun 2, 2015

seanmonstar commented Jun 2, 2015

mlalic commented Jun 3, 2015

mlalic commented Jun 3, 2015