feat(ext): support non-canonical HTTP/1 reason phrases #2792
Conversation
There was a problem hiding this comment.
So the tricky question is: is this worth a feature flag? Here are the performance numbers for the cargo bench suite (red is with the new feature disabled, green is with it enabled): https://gist.github.com/acfoltzer/c64e92ac95c390cb1c08624ecf61bfdf
It looks to me like roughly a wash, though there's a fair amount of noise in the sample. The problem is that none of the existing benches use the custom reason phrase, so it's hard to see the overall effect that existing users of "full" might get if they're already receiving responses with non-canonical phrases.
From an API cleanliness perspective I would prefer rolling this extension into the existing "http1" flag, but I also don't want to be introducing undue surprises for users of client who are not currently having to bear the cost of either the comparison against the canonical reason phrase or the allocation to store a custom one when sighted.
src/proto/h1/role.rs
Outdated
| #[cfg(feature = "http1_reason_phrase")] | ||
| let custom_reason_phrase = msg.head.extensions.get::<crate::ext::ReasonPhrase>(); | ||
| #[cfg(not(feature = "http1_reason_phrase"))] | ||
| #[allow(non_upper_case_globals)] | ||
| const custom_reason_phrase: Option<()> = None; |
There was a problem hiding this comment.
Here's where it starts getting a bit messy. To reduce the overall cfgness of this code, I'm relying on const evaluation to ensure this does not have an effect on performance when the flag is not enabled.
acfoltzer
force-pushed
the
acfoltzer/http1_reason_phrase
branch
from
a99c3a7
to
e7be2e1
Compare
|
@acfoltzer was this waiting on anything else, or just another review from me? |
|
@seanmonstar yep, though it looks like I'll need to fix some merge conflicts first. I'll try to get to that soon |
Add a new extension type `hyper::ext::ReasonPhrase` gated by either the `ffi` or `http1` Cargo features. When enabled, store any non-canonical reason phrases in this extension when parsing responses, and write this reason phrase instead of the canonical reason phrase when emitting responses. Reason phrases are a disused corner of the spec that implementations ought to treat as opaque blobs of bytes. Unfortunately, real-world traffic sometimes does depend on being able to inspect and manipulate them. Non-canonical reason phrases are checked for validity at runtime to prevent invalid and dangerous characters from being emitted when writing responses. An `unsafe` escape hatch is present for hyper itself to create reason phrases that have been parsed (and therefore implicitly validated) by httparse.
acfoltzer
force-pushed
the
acfoltzer/http1_reason_phrase
branch
from
4ca1eee
to
6ed3a7f
Compare
|
@seanmonstar rebased and squashed, incorporating the proposed change to fold this into the |
Add a new extension type
hyper::ext::ReasonPhrasegated by a new feature flaghttp1_reason_phrase. When enabled, store any non-canonical reason phrases in this extension when parsing responses, and write this reason phrase instead of the canonical reason phrase when emitting responses.Reason phrases are a disused corner of the spec that implementations ought to treat as opaque blobs of bytes. Unfortunately, real-world traffic sometimes does depend on being able to inspect and manipulate them.
My main outstanding question is: should this new feature flag be included in
full? Excluding it means this patch also has to modify the CI configuration in order to run theclientandservertests.