Fix #1067 by importing typed headers from hyperx

[jespersm]

First attempt patch for #1067.

Imports all typed headers from hyperx except "Set-Cookie" since it requires multiline format, and thus can't do simple to_string().

Probably not a big deal since Rocket handles cookies through CookieJar mechanism.

[jespersm]

I think this PR is ready for review now, @jebrosen :-)

I'm not entirely sure that the reexport of HyperxHeaderTrait is elegant enough, but there's a "Header" confusion to take into account.

[jebrosen]

I do like this, but I think we can organize it a bit more nicely.

One thing I did realize while reviewing this is that hyperx does not implement some of traits hyper uses it for headers, cc #1498. That's one point in favor of headers instead of hyperx that I did not consider, depending on how we prioritize #1498.

[jebrosen]

I think this is actually a bit weird, now that I'm seeing it. Can we try moving the headers parts into e.g. core/http/src/headers.rs instead?

[jebrosen]

Let's simply remove these header names re-exported from http. Typed headers should make most of them unneeded, and the only remaining benefit of import_http_headers was that applications don't need to depend on http directly.

There are a few places in rocket that use these, which used typed headers in 0.4 and could use them again.

[jebrosen]

This one isn't too bad a loss - SetCookie (and Cookie) is typically done via CookieJar.

[jebrosen]

This is needed in order to call HeaderName, right? As far as I can tell we did not re-export this trait in 0.4, but it does seem useful since it can be imported in the example. The name is a bit awkward, though.

[jespersm]

I’ll take another look at #1498 to see how this interacts.

[jespersm]

I've looked further at headers, which appears to use a HeaderValue struct from headers-core, which is yet another one, different one from the HeaderValue in hyper. That doesn't seem to line up with the change proposed in #1498, but I might not be Rust-y enough to know if there's a simple way to do that without cloning the underlying bytes.

[jebrosen]

I've looked further at headers, which appears to use a HeaderValue struct from headers-core, which is yet another one, different one from the HeaderValue in hyper. That doesn't seem to line up with the change proposed in #1498, but I might not be Rust-y enough to know if there's a simple way to do that without cloning the underlying bytes.

Both headers-core's and hyper's HeaderValue types are actually re-exports of http::headers::HeaderValue (https://docs.rs/headers-core/0.2.0/src/headers_core/lib.rs.html#13, https://docs.rs/hyper/0.14.4/src/hyper/lib.rs.html#73).

@SergioBenitez is #1498 enough of a concern that we should avoid using hyperx to provide typed headers, which currently has non-zero overhead to be converted to the HeaderValue type used by hyper?

[jespersm]

Both headers-core's and hyper's HeaderValue types are actually re-exports of http::headers::HeaderValue (https://docs.rs/headers-core/0.2.0/src/headers_core/lib.rs.html#13, https://docs.rs/hyper/0.14.4/src/hyper/lib.rs.html#73).

Ah, I was confused by a layer of turtles.

Anyway, I'm giving #1498 a shot to see how big an impact it would have.

[SergioBenitez]

@SergioBenitez is #1498 enough of a concern that we should avoid using hyperx to provide typed headers, which currently has non-zero overhead to be converted to the HeaderValue type used by hyper?

I believe this is the single largest source of slowdown in request handling. We should do nothing to exacerbate the issue or further prolong its mending.

[jebrosen]

I'm revisiting this and I am quite confused about where this ended up.

@jespersm, why did it matter whether or not hyperx implements HeaderValue, when we are targeting Header::new?

@SergioBenitez #1498 addressed handling of headers in the request, but didn't touch Responses which still unconditionally copies data as it did in 0.4 (https://github.com/SergioBenitez/Rocket/blob/master/core/lib/src/server.rs#L130). What needs to happen to implement this with the desired performance characteristics?

[SergioBenitez]

@SergioBenitez #1498 addressed handling of headers in the request, but didn't touch Responses which still unconditionally copies data as it did in 0.4 (https://github.com/SergioBenitez/Rocket/blob/master/core/lib/src/server.rs#L130). What needs to happen to implement this with the desired performance characteristics?

I don't think there's anything we can reasonably do about this, unfortunately. See hyperium/http#467.

Hopefully we get a fix upstream.

[SergioBenitez]

I am closing this due to lack of progress. Happy to revisit if all of the points raised above are addressed.