-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Body know about incoming Content-Length #1554
Conversation
2d69173
to
6f755b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for jumping on this! Just some minor thoughts left inline.
src/body/body.rs
Outdated
/// | ||
/// Useful when wanting to stream chunks from another thread. | ||
#[inline] | ||
pub fn channel_with_length(content_length: u64) -> (Sender, Body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'd suggest making this just pub(crate)
.
Oh and then, these 2 functions can probably be reduced to calling a 3rd, private constructor, fn new_channel(content_length: Option<u64>)
, to remove duplicate code, yea?
src/body/body.rs
Outdated
Async::Ready(Some(Ok(chunk))) => Ok(Async::Ready(Some(chunk))), | ||
Async::Ready(Some(Ok(chunk))) => { | ||
if let Some(ref mut len) = *len { | ||
match len.checked_sub(chunk.len() as u64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the constructor to set a length on the channel variant is private, we can rely on hyper's decoder to always limit the body size that is streamed (it enforces the length from the connection). Just for sanity, we could keep a debug assertion. Something like:
if let Some(ref mut len) = *len {
debug_assert!(*len >= chunk.len());
*len = *len - chunk.len();
}
e283968
to
2d885f7
Compare
2d885f7
to
b27ba1d
Compare
Done, I think. |
Thanks! |
Fixes #1545.
I'm not sure about: