From 31f117ea08c01889016fd45e7084e9a049c53f7a Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 5 Aug 2015 16:42:48 -0700 Subject: [PATCH 1/2] fix(client): improve HttpReader selection for client Responses Closes #436 --- src/header/common/content_length.rs | 113 ++++++++++++++++++++-------- src/header/common/mod.rs | 7 +- src/header/parsing.rs | 15 ++-- src/http/h1.rs | 48 +++++++----- 4 files changed, 122 insertions(+), 61 deletions(-) diff --git a/src/header/common/content_length.rs b/src/header/common/content_length.rs index 56cb9c54..f3b27011 100644 --- a/src/header/common/content_length.rs +++ b/src/header/common/content_length.rs @@ -1,37 +1,86 @@ -header! { - #[doc="`Content-Length` header, defined in"] - #[doc="[RFC7230](http://tools.ietf.org/html/rfc7230#section-3.3.2)"] - #[doc=""] - #[doc="When a message does not have a `Transfer-Encoding` header field, a"] - #[doc="Content-Length header field can provide the anticipated size, as a"] - #[doc="decimal number of octets, for a potential payload body. For messages"] - #[doc="that do include a payload body, the Content-Length field-value"] - #[doc="provides the framing information necessary for determining where the"] - #[doc="body (and message) ends. For messages that do not include a payload"] - #[doc="body, the Content-Length indicates the size of the selected"] - #[doc="representation."] - #[doc=""] - #[doc="# ABNF"] - #[doc="```plain"] - #[doc="Content-Length = 1*DIGIT"] - #[doc="```"] - #[doc=""] - #[doc="# Example values"] - #[doc="* `3495`"] - #[doc=""] - #[doc="# Example"] - #[doc="```"] - #[doc="use hyper::header::{Headers, ContentLength};"] - #[doc=""] - #[doc="let mut headers = Headers::new();"] - #[doc="headers.set(ContentLength(1024u64));"] - #[doc="```"] - (ContentLength, "Content-Length") => [u64] +use std::fmt; - test_content_length { - // Testcase from RFC - test_header!(test1, vec![b"3495"], Some(HeaderField(3495))); +use header::{HeaderFormat, Header, parsing}; + +#[doc="`Content-Length` header, defined in"] +#[doc="[RFC7230](http://tools.ietf.org/html/rfc7230#section-3.3.2)"] +#[doc=""] +#[doc="When a message does not have a `Transfer-Encoding` header field, a"] +#[doc="Content-Length header field can provide the anticipated size, as a"] +#[doc="decimal number of octets, for a potential payload body. For messages"] +#[doc="that do include a payload body, the Content-Length field-value"] +#[doc="provides the framing information necessary for determining where the"] +#[doc="body (and message) ends. For messages that do not include a payload"] +#[doc="body, the Content-Length indicates the size of the selected"] +#[doc="representation."] +#[doc=""] +#[doc="# ABNF"] +#[doc="```plain"] +#[doc="Content-Length = 1*DIGIT"] +#[doc="```"] +#[doc=""] +#[doc="# Example values"] +#[doc="* `3495`"] +#[doc=""] +#[doc="# Example"] +#[doc="```"] +#[doc="use hyper::header::{Headers, ContentLength};"] +#[doc=""] +#[doc="let mut headers = Headers::new();"] +#[doc="headers.set(ContentLength(1024u64));"] +#[doc="```"] +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct ContentLength(pub u64); + +impl Header for ContentLength { + #[inline] + fn header_name() -> &'static str { + "Content-Length" + } + fn parse_header(raw: &[Vec]) -> ::Result { + // If multiple Content-Length headers were sent, everything can still + // be alright if they all contain the same value, and all parse + // correctly. If not, then it's an error. + raw.iter() + .map(::std::ops::Deref::deref) + .map(parsing::from_raw_str) + .fold(None, |prev, x| { + match (prev, x) { + (None, x) => Some(x), + (e@Some(Err(_)), _ ) => e, + (Some(Ok(prev)), Ok(x)) if prev == x => Some(Ok(prev)), + _ => Some(Err(::Error::Header)) + } + }) + .unwrap_or(Err(::Error::Header)) + .map(ContentLength) } } +impl HeaderFormat for ContentLength { + #[inline] + fn fmt_header(&self, f: &mut fmt::Formatter) -> fmt::Result { + fmt::Display::fmt(&self.0, f) + } +} + +impl fmt::Display for ContentLength { + #[inline] + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fmt::Display::fmt(&self.0, f) + } +} + +__hyper__deref!(ContentLength => u64); +__hyper_generate_header_serialization!(ContentLength); + +__hyper__tm!(ContentLength, tests { + // Testcase from RFC + test_header!(test1, vec![b"3495"], Some(HeaderField(3495))); + + test_header!(test_invalid, vec![b"34v95"], None); + test_header!(test_duplicates, vec![b"5", b"5"], Some(HeaderField(5))); + test_header!(test_duplicates_vary, vec![b"5", b"6", b"5"], None); +}); + bench_header!(bench, ContentLength, { vec![b"42349984".to_vec()] }); diff --git a/src/header/common/mod.rs b/src/header/common/mod.rs index f1be3a05..6ef5c7fe 100644 --- a/src/header/common/mod.rs +++ b/src/header/common/mod.rs @@ -148,12 +148,13 @@ macro_rules! test_header { fn $id() { let a: Vec> = $raw.iter().map(|x| x.to_vec()).collect(); let val = HeaderField::parse_header(&a[..]); + let typed: Option = $typed; // Test parsing - assert_eq!(val.ok(), $typed); + assert_eq!(val.ok(), typed); // Test formatting - if $typed != None { + if typed.is_some() { let res: &str = str::from_utf8($raw[0]).unwrap(); - assert_eq!(format!("{}", $typed.unwrap()), res); + assert_eq!(format!("{}", typed.unwrap()), res); } } } diff --git a/src/header/parsing.rs b/src/header/parsing.rs index 5a4e5728..15ce8ecc 100644 --- a/src/header/parsing.rs +++ b/src/header/parsing.rs @@ -3,16 +3,17 @@ use std::str; use std::fmt::{self, Display}; -/// Reads a single raw string when parsing a header +/// Reads a single raw string when parsing a header. pub fn from_one_raw_str(raw: &[Vec]) -> ::Result { if raw.len() != 1 || unsafe { raw.get_unchecked(0) } == b"" { return Err(::Error::Header) } // we JUST checked that raw.len() == 1, so raw[0] WILL exist. - let s: &str = try!(str::from_utf8(& unsafe { raw.get_unchecked(0) }[..])); - if let Ok(x) = str::FromStr::from_str(s) { - Ok(x) - } else { - Err(::Error::Header) - } + from_raw_str(& unsafe { raw.get_unchecked(0) }) +} + +/// Reads a raw string into a value. +pub fn from_raw_str(raw: &[u8]) -> ::Result { + let s = try!(str::from_utf8(raw)); + T::from_str(s).or(Err(::Error::Header)) } /// Reads a comma-delimited raw header into a Vec. diff --git a/src/http/h1.rs b/src/http/h1.rs index fbc830a0..71d3a232 100644 --- a/src/http/h1.rs +++ b/src/http/h1.rs @@ -36,6 +36,7 @@ use version; /// An implementation of the `HttpMessage` trait for HTTP/1.1. #[derive(Debug)] pub struct Http11Message { + method: Option, stream: Option>, writer: Option>>>, reader: Option>>>, @@ -91,8 +92,8 @@ impl HttpMessage for Http11Message { try!(write!(&mut stream, "{} {} {}{}", head.method, uri, version, LINE_ENDING)); - let stream = match head.method { - Method::Get | Method::Head => { + let stream = match &head.method { + &Method::Get | &Method::Head => { debug!("headers={:?}", head.headers); try!(write!(&mut stream, "{}{}", head.headers, LINE_ENDING)); EmptyWriter(stream) @@ -137,6 +138,7 @@ impl HttpMessage for Http11Message { } }; + self.method = Some(head.method.clone()); self.writer = Some(stream); Ok(head) @@ -159,30 +161,37 @@ impl HttpMessage for Http11Message { let raw_status = head.subject; let headers = head.headers; - let body = if headers.has::() { - match headers.get::() { - Some(&TransferEncoding(ref codings)) => { - if codings.len() > 1 { - trace!("TODO: #2 handle other codings: {:?}", codings); - }; - - if codings.contains(&Chunked) { + let method = self.method.take().unwrap_or(Method::Get); + // According to https://tools.ietf.org/html/rfc7230#section-3.3.3 + // 1. HEAD reponses, and Status 1xx, 204, and 304 cannot have a body. + // 2. Status 2xx to a CONNECT cannot have a body. + // 3. Transfer-Encoding: chunked has a chunked body. + // 4. If multiple differing Content-Length headers or invalid, close connection. + // 5. Content-Length header has a sized body. + // 6. Not Client. + // 7. Read till EOF. + let body = match (method, raw_status.0) { + (Method::Head, _) => EmptyReader(stream), + (_, 100...199) | (_, 204) | (_, 304) => EmptyReader(stream), + (Method::Connect, 200...299) => EmptyReader(stream), + _ => { + if let Some(&TransferEncoding(ref codings)) = headers.get() { + if codings.last() == Some(&Chunked) { ChunkedReader(stream, None) } else { trace!("not chuncked. read till eof"); EofReader(stream) } + } else if let Some(&ContentLength(len)) = headers.get() { + SizedReader(stream, len) + } else if headers.has::() { + trace!("illegal Content-Length: {:?}", headers.get_raw("Content-Length")); + return Err(Error::Header); + } else { + trace!("neither Transfer-Encoding nor Content-Length"); + EofReader(stream) } - None => unreachable!() } - } else if headers.has::() { - match headers.get::() { - Some(&ContentLength(len)) => SizedReader(stream, len), - None => unreachable!() - } - } else { - trace!("neither Transfer-Encoding nor Content-Length"); - EofReader(stream) }; self.reader = Some(body); @@ -259,6 +268,7 @@ impl Http11Message { /// the peer. pub fn with_stream(stream: Box) -> Http11Message { Http11Message { + method: None, stream: Some(stream), writer: None, reader: None, From 67c284a96a006f888f43d8af929516465de76dea Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 5 Aug 2015 16:45:54 -0700 Subject: [PATCH 2/2] fix(client): improve keep-alive of bodyless Responses --- src/client/response.rs | 4 ++-- src/http/h1.rs | 7 +++++++ src/http/h2.rs | 4 ++++ src/http/message.rs | 2 ++ 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/client/response.rs b/src/client/response.rs index 44684eda..46882d42 100644 --- a/src/client/response.rs +++ b/src/client/response.rs @@ -47,9 +47,9 @@ impl Response { version: version, headers: headers, url: url, - message: message, status_raw: raw_status, - is_drained: false, + is_drained: !message.has_body(), + message: message, }) } diff --git a/src/http/h1.rs b/src/http/h1.rs index 71d3a232..44cd35db 100644 --- a/src/http/h1.rs +++ b/src/http/h1.rs @@ -203,6 +203,13 @@ impl HttpMessage for Http11Message { }) } + fn has_body(&self) -> bool { + match self.reader { + Some(EmptyReader(..)) => false, + _ => true + } + } + #[cfg(feature = "timeouts")] #[inline] fn set_read_timeout(&self, dur: Option) -> io::Result<()> { diff --git a/src/http/h2.rs b/src/http/h2.rs index 84af8ff5..136bf80c 100644 --- a/src/http/h2.rs +++ b/src/http/h2.rs @@ -400,6 +400,10 @@ impl HttpMessage for Http2Message where S: CloneableStream { Ok(head) } + fn has_body(&self) -> bool { + true + } + #[cfg(feature = "timeouts")] #[inline] fn set_read_timeout(&self, _dur: Option) -> io::Result<()> { diff --git a/src/http/message.rs b/src/http/message.rs index f0f2d9b2..b969d314 100644 --- a/src/http/message.rs +++ b/src/http/message.rs @@ -72,6 +72,8 @@ pub trait HttpMessage: Write + Read + Send + Any + Typeable + Debug { fn set_write_timeout(&self, dur: Option) -> io::Result<()>; /// Closes the underlying HTTP connection. fn close_connection(&mut self) -> ::Result<()>; + /// Returns whether the incoming message has a body. + fn has_body(&self) -> bool; } impl HttpMessage {