Get content-length for Incoming HTTP2 Bodies #1557
Conversation
src/body/body.rs
Outdated
| @@ -39,7 +39,7 @@ enum Kind { | |||
| abort_rx: oneshot::Receiver<()>, | |||
| rx: mpsc::Receiver<Result<Chunk, ::Error>>, | |||
| }, | |||
| H2(h2::RecvStream), | |||
| H2(h2::RecvStream, Option<u64>), | |||
There was a problem hiding this comment.
Nit-pick: maybe swap these and make it a struct-like variant (H2 { ... })
There was a problem hiding this comment.
Looking good!
There were a couple tests added in #1556 with lines commented out waiting for this change, want to uncomment them in this PR so we have tests for this functionality?
src/body/body.rs
Outdated
| }) | ||
| } | ||
| Kind::H2 { | ||
| content_length: ref mut _len, |
There was a problem hiding this comment.
Since it's not needed, this pattern could be replace with just ...
src/proto/h2/client.rs
Outdated
| @@ -110,6 +111,7 @@ where | |||
| if let Some(len) = body.content_length() { | |||
| headers::set_content_length_if_missing(req.headers_mut(), len); | |||
| } | |||
| let content_length = content_length_parse_all(req.headers()); | |||
There was a problem hiding this comment.
This looks like it sets content_length of the Response's body by parsing the length from the Request. (Curious the tests didn't catch it...)
There was a problem hiding this comment.
It looks like it does pick it up but I suspect that because the panic is not in the main test thread it doesn't cause the test to fail.
https://travis-ci.org/hyperium/hyper/jobs/390534326#L825
https://travis-ci.org/hyperium/hyper/jobs/390534326#L825
|
Sorry, it seems github doesn't send notifications about rebased/ammended commits. I'll look now. |
There was a problem hiding this comment.
Awesome, looks great!
Seems merging the upgrade PR made this have merge conflicts, but I suspect that aren't too bad. It's probably just around the poll_eof method, mostly because this PR reformats it (I'd personally recommend not reformating unrelated changes because of the risk of conflicts :D).
Add `Body::Kind::H2` to contain the content length of the body. Closes: #1546
Update `Body::content_length` to return the content length if `Body::Kind` is `H2`, instead of returning `None`.
Refactor the `Kind::H2` enum variant to be a named variant, rather than an unnamed variant. This is to make fields, currently `recv: h2::RecvStream`, and `content_length: Option<u64>` more readable.
|
Ahh damn sorry about that. Should be all fixed now 👍 |
|
Thanks! |
Update
Body::content_lengthto return the content length of whenBody::KindisH2, instead of returningNone.Kind::H2also holds the value of thecontent-lengthheader as well as theh2::RecvStream.Closes: #1546