fix(ffi): Change hyper_headers_foreach to iterate in recieved order #2798

liamwarfield · 2022-03-30T00:52:50Z

Libcurl expects that headers are iterated in the same order that they
are recieved. Previously this caused curl tests 580 and 581 to fail.

SUMMARY OF CHANGES: Add a new data structure to HeaderCaseMap that
represents the order in which headers originally appear in a HTTP
message. This datastructure is Vec<(Headername, multimap-index)>.
This vector is ordered by the order which headers were recieved.
This new datastucture is used in hyper_headers_foreach to iterate
through headers in the correct order.

CLOSES ISSUE: #2780
BEAKING CHANGE: HeaderCaseMap::append now requires that names passed
into it impl Into + Clone. Currently all types that impl
IntoHeaderName (in hyper and http) also impl these traits already.

liamwarfield · 2022-03-30T00:54:04Z

There a few things that still need to be done here (like testing), but this is here so that I can get some comments.

seanmonstar · 2022-03-30T02:27:29Z

Thanks for working on this! A couple of thoughts:

Since the HeaderCaseMap is used by things that don't need this specific functionality, I think it's best to create a separate type.
It's probably even a good idea to make this not the default in the C API, but make it a connection option.

I can explain more about either point if you want.

liamwarfield · 2022-03-30T14:39:53Z

I think you're right about breaking things out into another type. As for how to separate this out in the c api, how does this sound:

add HeaderOrder type to hyper_headers

pub struct hyper_headers {
    pub(super) headers: HeaderMap,
    orig_casing: HeaderCaseMap,
    orig_order: OriginalHeaderOrder,
}

Add a new function called hyper_headers_foreach_ordered, and revert hyper_headers_foreach to its original implementation.

Could you elaborate on how connection options work?
Are there any performance tests in hyper?

liamwarfield · 2022-03-30T16:15:51Z

I did some digging into the other places that HeaderCaseMap is used. Its mostly used in the h1 to represent the original headers received.

From a memory overhead prospective, HeaderCaseMap is only tacked on to a request's Extensions if preserve_header_case == true, so adding more data HeaderCaseMap should not impact most users.
From a CPU time perspective, HeaderCaseMap is only really used when parsing and encoding h1 requests. Both of those code paths are marked as cold.

hyper/src/proto/h1/role.rs

Line 512 in 53f15e5

fn encode_headers_with_original_case(

hyper/src/proto/h1/role.rs

Line 1405 in 53f15e5

fn write_headers_original_case(

Does this change anything about you opinion here?

As far as making a new connection option goes, how does does something along the lines of preserve_header_order?

liamwarfield · 2022-03-31T03:22:52Z

Just pushed up a commit for adding a new connection option. My plan is to squash all my commits once I have something that we want to merge.

seanmonstar · 2022-03-31T19:43:49Z

Awesome, I'll review this afternoon. The other uses so far are proxies that enable the option, and then pass the extension on to an inner service. For instance, @nox did some work to make that possible for a proxy he supports. There is an issue filed here about exposing the extension publicly to make it more available.

seanmonstar

Hey, this is really good work. Really! ❤️

I left some thoughts inline.

src/ext.rs

src/client/conn.rs

src/ffi/client.rs

src/ext.rs

Libcurl expects that headers are iterated in the same order that they are recieved. Previously this caused curl tests 580 and 581 to fail. This necessitated exposing a way to preserve the original ordering of http headers. SUMMARY OF CHANGES: Add a new data structure called OriginalHeaderOrder that represents the order in which headers originally appear in a HTTP message. This datastructure is `Vec<(Headername, multimap-index)>`. This vector is ordered by the order which headers were recieved. Add the following ffi functions: - ffi::client::hyper_clientconn_options_set_preserve_header_order : An ffi interface to configure a connection to preserve header order. - ffi::client::hyper_clientconn_options_set_preserve_header_case : An ffi interface to configure a connection to preserve header case. - ffi::http_types::hyper_headers_foreach_ordered : Iterates the headers in the order the were recieved, passing each name and value pair to the callback. Add a new option to ParseContext, and Conn::State called `preserve_header_order`. This option, and all the code paths it creates are behind the `ffi` feature flag. This should not change performance of response parsing for non-ffi users. CLOSES ISSUE: hyperium#2780

liamwarfield

Did a quick review while squashing.

src/ffi/client.rs

src/ffi/http_types.rs

…ders_foreach_ordered hyper_headers_foreach_ordered was merged into hyper_headers_foreach. OriginalHeaderOrder docs were given examples OriginalHeaderOrder has named members for better clarity. BREAKING CHANGE: ffi::client::hyper_clientconn_options_new no longer sets the http1_preserve_header_case connection option by default. Users should now call ffi::client::hyper_clientconn_options_set_preserve_header_order if they desire that functionality

Hyper will have be able to preserve header order once this PR merges: hyperium/hyper#2798 This commit adds a few lines setting the connection options for this feature. Related to issue curl#8617

liamwarfield · 2022-04-14T04:48:25Z

Just pushed up a patch, addressing the last round of comments.

seanmonstar

Awesome work, this looks really good. Thank you!

Just needs a tiny change to fix the unnused mut warning, and I think we can merge.

seanmonstar · 2022-04-21T19:26:29Z

src/ffi/client.rs

@@ -94,7 +94,6 @@ ffi_fn! {
    /// Creates a new set of HTTP clientconn options to be used in a handshake.
    fn hyper_clientconn_options_new() -> *mut hyper_clientconn_options {
        let mut builder = conn::Builder::new();
-        builder.http1_preserve_header_case(true);


Looks like this removal makes a lint trigger about no longer needing to be let mut builder.

seanmonstar · 2022-04-21T19:29:53Z

This isn't a requirement, but something that I'm slightly curious about: when this cfg is enabled (but without setting the builder option), does it make a performance difference? Such as running cargo bench --bench end_to_end, or --bench pipeline? 🤷

liamwarfield · 2022-04-21T20:57:18Z

I'll look into the performance stuff when I get some time later.

seanmonstar · 2022-04-21T21:19:19Z

I'm also happy to just merge with the simple lint fix. Whatever you like :)

This cased the CI to fail

seanmonstar · 2022-04-23T16:06:07Z

Superb work, thank you thank you! ❤️

liamwarfield · 2022-04-25T17:18:16Z

The Curl Commit to enable and fix tests 580 and 581 has been merged!

curl/curl#8707

liamwarfield mentioned this pull request Apr 7, 2022

Propose the 1.0 Roadmap #2806

Merged

seanmonstar requested changes Apr 8, 2022

View reviewed changes

src/ext.rs Outdated Show resolved Hide resolved

src/client/conn.rs Show resolved Hide resolved

src/ffi/client.rs Outdated Show resolved Hide resolved

src/ext.rs Outdated Show resolved Hide resolved

liamwarfield commented Apr 11, 2022

View reviewed changes

src/ffi/client.rs Outdated Show resolved Hide resolved

src/ffi/http_types.rs Outdated Show resolved Hide resolved

liamwarfield force-pushed the lwarfield/header-reordering-issue branch from 0ff3b20 to f4fec5c Compare April 11, 2022 18:23

Merge branch 'master' into lwarfield/header-reordering-issue

3154fbb

liamwarfield requested a review from seanmonstar April 11, 2022 18:33

liamwarfield mentioned this pull request Apr 14, 2022

hyper: Fixxed tests 580 and 581 for the hyper backend curl/curl#8707

Closed

seanmonstar approved these changes Apr 21, 2022

View reviewed changes

missed a few #[cfg(feature = "ffi")]

fcc9851

This cased the CI to fail

liamwarfield force-pushed the lwarfield/header-reordering-issue branch from 9d71608 to fcc9851 Compare April 22, 2022 22:31

seanmonstar merged commit 78de891 into hyperium:master Apr 23, 2022

This was linked to issues Apr 28, 2022

Header order gets rearranged for duplicated header names #2572

Closed

Hyper Curl test 580 and 580 Root cause #2780

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ffi): Change hyper_headers_foreach to iterate in recieved order #2798

fix(ffi): Change hyper_headers_foreach to iterate in recieved order #2798

liamwarfield commented Mar 30, 2022

liamwarfield commented Mar 30, 2022

seanmonstar commented Mar 30, 2022

liamwarfield commented Mar 30, 2022 •

edited

liamwarfield commented Mar 30, 2022

liamwarfield commented Mar 31, 2022 •

edited

seanmonstar commented Mar 31, 2022

seanmonstar left a comment

liamwarfield left a comment

liamwarfield commented Apr 14, 2022

seanmonstar left a comment

seanmonstar Apr 21, 2022

seanmonstar commented Apr 21, 2022

liamwarfield commented Apr 21, 2022

seanmonstar commented Apr 21, 2022

seanmonstar commented Apr 23, 2022

liamwarfield commented Apr 25, 2022

fix(ffi): Change hyper_headers_foreach to iterate in recieved order #2798

fix(ffi): Change hyper_headers_foreach to iterate in recieved order #2798

Conversation

liamwarfield commented Mar 30, 2022

liamwarfield commented Mar 30, 2022

seanmonstar commented Mar 30, 2022

liamwarfield commented Mar 30, 2022 • edited

liamwarfield commented Mar 30, 2022

liamwarfield commented Mar 31, 2022 • edited

seanmonstar commented Mar 31, 2022

seanmonstar left a comment

Choose a reason for hiding this comment

liamwarfield left a comment

Choose a reason for hiding this comment

liamwarfield commented Apr 14, 2022

seanmonstar left a comment

Choose a reason for hiding this comment

seanmonstar Apr 21, 2022

Choose a reason for hiding this comment

seanmonstar commented Apr 21, 2022

liamwarfield commented Apr 21, 2022

seanmonstar commented Apr 21, 2022

seanmonstar commented Apr 23, 2022

liamwarfield commented Apr 25, 2022

liamwarfield commented Mar 30, 2022 •

edited

liamwarfield commented Mar 31, 2022 •

edited