fix(client): improve HttpReader selection for client Responses

Closes #436

Author: seanmonstar

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]
-
-    test_content_length {
-        // Testcase from RFC
-        test_header!(test1, vec![b"3495"], Some(HeaderField(3495)));
+use std::fmt;
+
+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<u8>]) -> ::Result<ContentLength> {
+        // 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()] });

src/header/common/mod.rs

@@ -148,12 +148,13 @@ macro_rules! test_header {
         fn $id() {
             let a: Vec<Vec<u8>> = $raw.iter().map(|x| x.to_vec()).collect();
             let val = HeaderField::parse_header(&a[..]);
+            let typed: Option<HeaderField> = $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);
             }
         }
     }

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<T: str::FromStr>(raw: &[Vec<u8>]) -> ::Result<T> {
     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<T: str::FromStr>(raw: &[u8]) -> ::Result<T> {
+    let s = try!(str::from_utf8(raw));
+    T::from_str(s).or(Err(::Error::Header))
 }
 
 /// Reads a comma-delimited raw header into a Vec.

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<Method>,
     stream: Option<Box<NetworkStream + Send>>,
     writer: Option<HttpWriter<BufWriter<Box<NetworkStream + Send>>>>,
     reader: Option<HttpReader<BufReader<Box<NetworkStream + Send>>>>,
@@ -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::<TransferEncoding>() {
-            match headers.get::<TransferEncoding>() {
-                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::<ContentLength>() {
+                    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::<ContentLength>() {
-            match headers.get::<ContentLength>() {
-                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<NetworkStream + Send>) -> Http11Message {
         Http11Message {
+            method: None,
             stream: Some(stream),
             writer: None,
             reader: None,