fix(server): improve detection of when a Response can have a body
By knowing if the incoming Request was a HEAD, or checking for 204 or 304 status codes, the server will do a better job of either adding or removing `Content-Length` and `Transfer-Encoding` headers. Closes #1257
This commit is contained in:
		| @@ -7,10 +7,10 @@ use futures::task::Task; | ||||
| use tokio_io::{AsyncRead, AsyncWrite}; | ||||
| use tokio_proto::streaming::pipeline::{Frame, Transport}; | ||||
|  | ||||
| use header::{ContentLength, TransferEncoding}; | ||||
| use http::{self, Http1Transaction, DebugTruncate}; | ||||
| use http::io::{Cursor, Buffered}; | ||||
| use http::h1::{Encoder, Decoder}; | ||||
| use method::Method; | ||||
| use version::HttpVersion; | ||||
|  | ||||
|  | ||||
| @@ -37,10 +37,11 @@ where I: AsyncRead + AsyncWrite, | ||||
|         Conn { | ||||
|             io: Buffered::new(io), | ||||
|             state: State { | ||||
|                 keep_alive: keep_alive, | ||||
|                 method: None, | ||||
|                 read_task: None, | ||||
|                 reading: Reading::Init, | ||||
|                 writing: Writing::Init, | ||||
|                 read_task: None, | ||||
|                 keep_alive: keep_alive, | ||||
|             }, | ||||
|             _marker: PhantomData, | ||||
|         } | ||||
| @@ -103,7 +104,7 @@ where I: AsyncRead + AsyncWrite, | ||||
|  | ||||
|         match version { | ||||
|             HttpVersion::Http10 | HttpVersion::Http11 => { | ||||
|                 let decoder = match T::decoder(&head) { | ||||
|                 let decoder = match T::decoder(&head, &mut self.state.method) { | ||||
|                     Ok(d) => d, | ||||
|                     Err(e) => { | ||||
|                         debug!("decoder error = {:?}", e); | ||||
| @@ -234,17 +235,8 @@ where I: AsyncRead + AsyncWrite, | ||||
|         } | ||||
|     } | ||||
|  | ||||
|     fn write_head(&mut self, mut head: http::MessageHead<T::Outgoing>, body: bool) { | ||||
|     fn write_head(&mut self, head: http::MessageHead<T::Outgoing>, body: bool) { | ||||
|         debug_assert!(self.can_write_head()); | ||||
|         if !body { | ||||
|             head.headers.remove::<TransferEncoding>(); | ||||
|             //TODO: check that this isn't a response to a HEAD | ||||
|             //request, which could include the content-length | ||||
|             //even if no body is to be written | ||||
|             if T::should_set_length(&head) { | ||||
|                 head.headers.set(ContentLength(0)); | ||||
|             } | ||||
|         } | ||||
|  | ||||
|         let wants_keep_alive = head.should_keep_alive(); | ||||
|         self.state.keep_alive &= wants_keep_alive; | ||||
| @@ -256,8 +248,8 @@ where I: AsyncRead + AsyncWrite, | ||||
|                 buf.extend_from_slice(pending.buf()); | ||||
|             } | ||||
|         } | ||||
|         let encoder = T::encode(head, buf); | ||||
|         self.state.writing = if body { | ||||
|         let encoder = T::encode(head, body, &mut self.state.method, buf); | ||||
|         self.state.writing = if !encoder.is_eof() { | ||||
|             Writing::Body(encoder, None) | ||||
|         } else { | ||||
|             Writing::KeepAlive | ||||
| @@ -493,10 +485,11 @@ impl<I, B: AsRef<[u8]>, T, K: fmt::Debug> fmt::Debug for Conn<I, B, T, K> { | ||||
| } | ||||
|  | ||||
| struct State<B, K> { | ||||
|     keep_alive: K, | ||||
|     method: Option<Method>, | ||||
|     read_task: Option<Task>, | ||||
|     reading: Reading, | ||||
|     writing: Writing<B>, | ||||
|     read_task: Option<Task>, | ||||
|     keep_alive: K, | ||||
| } | ||||
|  | ||||
| #[derive(Debug)] | ||||
| @@ -522,6 +515,7 @@ impl<B: AsRef<[u8]>, K: fmt::Debug> fmt::Debug for State<B, K> { | ||||
|             .field("reading", &self.reading) | ||||
|             .field("writing", &self.writing) | ||||
|             .field("keep_alive", &self.keep_alive) | ||||
|             .field("method", &self.method) | ||||
|             .field("read_task", &self.read_task) | ||||
|             .finish() | ||||
|     } | ||||
| @@ -641,6 +635,7 @@ impl<B, K: KeepAlive> State<B, K> { | ||||
|     } | ||||
|  | ||||
|     fn idle(&mut self) { | ||||
|         self.method = None; | ||||
|         self.reading = Reading::Init; | ||||
|         self.writing = Writing::Init; | ||||
|         self.keep_alive.idle(); | ||||
|   | ||||
| @@ -5,7 +5,8 @@ use httparse; | ||||
| use bytes::{BytesMut, Bytes}; | ||||
|  | ||||
| use header::{self, Headers, ContentLength, TransferEncoding}; | ||||
| use http::{MessageHead, RawStatus, Http1Transaction, ParseResult, ServerTransaction, ClientTransaction, RequestLine}; | ||||
| use http::{MessageHead, RawStatus, Http1Transaction, ParseResult, | ||||
|            ServerTransaction, ClientTransaction, RequestLine, RequestHead}; | ||||
| use http::h1::{Encoder, Decoder, date}; | ||||
| use method::Method; | ||||
| use status::StatusCode; | ||||
| @@ -72,8 +73,11 @@ impl Http1Transaction for ServerTransaction { | ||||
|         }, len))) | ||||
|     } | ||||
|  | ||||
|     fn decoder(head: &MessageHead<Self::Incoming>) -> ::Result<Decoder> { | ||||
|     fn decoder(head: &MessageHead<Self::Incoming>, method: &mut Option<Method>) -> ::Result<Decoder> { | ||||
|         use ::header; | ||||
|  | ||||
|         *method = Some(head.subject.0.clone()); | ||||
|  | ||||
|         // According to https://tools.ietf.org/html/rfc7230#section-3.3.3 | ||||
|         // 1. (irrelevant to Request) | ||||
|         // 2. (irrelevant to Request) | ||||
| @@ -105,30 +109,11 @@ impl Http1Transaction for ServerTransaction { | ||||
|     } | ||||
|  | ||||
|  | ||||
|     fn encode(mut head: MessageHead<Self::Outgoing>, dst: &mut Vec<u8>) -> Encoder { | ||||
|         use ::header; | ||||
|         trace!("writing head: {:?}", head); | ||||
|     fn encode(mut head: MessageHead<Self::Outgoing>, has_body: bool, method: &mut Option<Method>, dst: &mut Vec<u8>) -> Encoder { | ||||
|         trace!("ServerTransaction::encode head={:?}, has_body={}, method={:?}", | ||||
|                head, has_body, method); | ||||
|  | ||||
|         let len = head.headers.get::<header::ContentLength>().map(|n| **n); | ||||
|  | ||||
|         let body = if let Some(len) = len { | ||||
|             Encoder::length(len) | ||||
|         } else { | ||||
|             let encodings = match head.headers.get_mut::<header::TransferEncoding>() { | ||||
|                 Some(&mut header::TransferEncoding(ref mut encodings)) => { | ||||
|                     if encodings.last() != Some(&header::Encoding::Chunked) { | ||||
|                         encodings.push(header::Encoding::Chunked); | ||||
|                     } | ||||
|                     false | ||||
|                 }, | ||||
|                 None => true | ||||
|             }; | ||||
|  | ||||
|             if encodings { | ||||
|                 head.headers.set(header::TransferEncoding(vec![header::Encoding::Chunked])); | ||||
|             } | ||||
|             Encoder::chunked() | ||||
|         }; | ||||
|         let body = ServerTransaction::set_length(&mut head, has_body, method.as_ref()); | ||||
|         debug!("encode headers = {:?}", head.headers); | ||||
|  | ||||
|         let init_cap = 30 + head.headers.len() * AVERAGE_HEADER_SIZE; | ||||
| @@ -150,16 +135,39 @@ impl Http1Transaction for ServerTransaction { | ||||
|         extend(dst, b"\r\n"); | ||||
|         body | ||||
|     } | ||||
| } | ||||
|  | ||||
|     fn should_set_length(head: &MessageHead<Self::Outgoing>) -> bool { | ||||
|         //TODO: pass method, check if method == HEAD | ||||
| impl ServerTransaction { | ||||
|     fn set_length(head: &mut MessageHead<StatusCode>, has_body: bool, method: Option<&Method>) -> Encoder { | ||||
|         // these are here thanks to borrowck | ||||
|         // `if method == Some(&Method::Get)` says the RHS doesnt live long enough | ||||
|         const HEAD: Option<&'static Method> = Some(&Method::Head); | ||||
|         const CONNECT: Option<&'static Method> = Some(&Method::Connect); | ||||
|  | ||||
|         match head.subject { | ||||
|             // TODO: support for 1xx codes needs improvement everywhere | ||||
|             // would be 100...199 => false | ||||
|             StatusCode::NoContent | | ||||
|             StatusCode::NotModified => false, | ||||
|             _ => true, | ||||
|         let can_have_body = { | ||||
|             if method == HEAD { | ||||
|                 false | ||||
|             } else if method == CONNECT && head.subject.is_success() { | ||||
|                 false | ||||
|             } else { | ||||
|                 match head.subject { | ||||
|                     // TODO: support for 1xx codes needs improvement everywhere | ||||
|                     // would be 100...199 => false | ||||
|                     StatusCode::NoContent | | ||||
|                     StatusCode::NotModified => false, | ||||
|                     _ => true, | ||||
|                 } | ||||
|             } | ||||
|         }; | ||||
|  | ||||
|         if has_body && can_have_body { | ||||
|             set_length(&mut head.headers) | ||||
|         } else { | ||||
|             head.headers.remove::<TransferEncoding>(); | ||||
|             if can_have_body { | ||||
|                 head.headers.set(ContentLength(0)); | ||||
|             } | ||||
|             Encoder::length(0) | ||||
|         } | ||||
|     } | ||||
| } | ||||
| @@ -213,8 +221,7 @@ impl Http1Transaction for ClientTransaction { | ||||
|         }, len))) | ||||
|     } | ||||
|  | ||||
|     fn decoder(inc: &MessageHead<Self::Incoming>) -> ::Result<Decoder> { | ||||
|         use ::header; | ||||
|     fn decoder(inc: &MessageHead<Self::Incoming>, method: &mut Option<Method>) -> ::Result<Decoder> { | ||||
|         // According to https://tools.ietf.org/html/rfc7230#section-3.3.3 | ||||
|         // 1. HEAD responses, and Status 1xx, 204, and 304 cannot have a body. | ||||
|         // 2. Status 2xx to a CONNECT cannot have a body. | ||||
| @@ -224,7 +231,21 @@ impl Http1Transaction for ClientTransaction { | ||||
|         // 6. (irrelevant to Response) | ||||
|         // 7. Read till EOF. | ||||
|  | ||||
|         //TODO: need a way to pass the Method that caused this Response | ||||
|         match *method { | ||||
|             Some(Method::Head) => { | ||||
|                 return Ok(Decoder::length(0)); | ||||
|             } | ||||
|             Some(Method::Connect) => match inc.subject.0 { | ||||
|                 200...299 => { | ||||
|                     return Ok(Decoder::length(0)); | ||||
|                 }, | ||||
|                 _ => {}, | ||||
|             }, | ||||
|             Some(_) => {}, | ||||
|             None => { | ||||
|                 trace!("ClientTransaction::decoder is missing the Method"); | ||||
|             } | ||||
|         } | ||||
|  | ||||
|         match inc.subject.0 { | ||||
|             100...199 | | ||||
| @@ -251,39 +272,14 @@ impl Http1Transaction for ClientTransaction { | ||||
|         } | ||||
|     } | ||||
|  | ||||
|     fn encode(mut head: MessageHead<Self::Outgoing>, dst: &mut Vec<u8>) -> Encoder { | ||||
|         trace!("writing head: {:?}", head); | ||||
|     fn encode(mut head: MessageHead<Self::Outgoing>, has_body: bool, method: &mut Option<Method>, dst: &mut Vec<u8>) -> Encoder { | ||||
|         trace!("ClientTransaction::encode head={:?}, has_body={}, method={:?}", | ||||
|                head, has_body, method); | ||||
|  | ||||
|  | ||||
|         let mut body = Encoder::length(0); | ||||
|         let expects_no_body = match head.subject.0 { | ||||
|             Method::Head | Method::Get | Method::Connect => true, | ||||
|             _ => false | ||||
|         }; | ||||
|         let mut chunked = false; | ||||
|         *method = Some(head.subject.0.clone()); | ||||
|  | ||||
|         if let Some(con_len) = head.headers.get::<ContentLength>() { | ||||
|             body = Encoder::length(**con_len); | ||||
|         } else { | ||||
|             chunked = !expects_no_body; | ||||
|         } | ||||
|  | ||||
|         if chunked { | ||||
|             body = Encoder::chunked(); | ||||
|             let encodings = match head.headers.get_mut::<TransferEncoding>() { | ||||
|                 Some(encodings) => { | ||||
|                     if !encodings.contains(&header::Encoding::Chunked) { | ||||
|                         encodings.push(header::Encoding::Chunked); | ||||
|                     } | ||||
|                     true | ||||
|                 }, | ||||
|                 None => false | ||||
|             }; | ||||
|  | ||||
|             if !encodings { | ||||
|                 head.headers.set(TransferEncoding(vec![header::Encoding::Chunked])); | ||||
|             } | ||||
|         } | ||||
|         let body = ClientTransaction::set_length(&mut head, has_body); | ||||
|         debug!("encode headers = {:?}", head.headers); | ||||
|  | ||||
|         let init_cap = 30 + head.headers.len() * AVERAGE_HEADER_SIZE; | ||||
| @@ -292,16 +288,43 @@ impl Http1Transaction for ClientTransaction { | ||||
|  | ||||
|         body | ||||
|     } | ||||
| } | ||||
|  | ||||
|  | ||||
|     fn should_set_length(head: &MessageHead<Self::Outgoing>) -> bool { | ||||
|         match &head.subject.0 { | ||||
|             &Method::Get | &Method::Head => false, | ||||
|             _ => true | ||||
| impl ClientTransaction { | ||||
|     fn set_length(head: &mut RequestHead, has_body: bool) -> Encoder { | ||||
|         if has_body { | ||||
|             set_length(&mut head.headers) | ||||
|         } else { | ||||
|             head.headers.remove::<ContentLength>(); | ||||
|             head.headers.remove::<TransferEncoding>(); | ||||
|             Encoder::length(0) | ||||
|         } | ||||
|     } | ||||
| } | ||||
|  | ||||
| fn set_length(headers: &mut Headers) -> Encoder { | ||||
|     let len = headers.get::<header::ContentLength>().map(|n| **n); | ||||
|  | ||||
|     if let Some(len) = len { | ||||
|         Encoder::length(len) | ||||
|     } else { | ||||
|         let encodings = match headers.get_mut::<header::TransferEncoding>() { | ||||
|             Some(&mut header::TransferEncoding(ref mut encodings)) => { | ||||
|                 if encodings.last() != Some(&header::Encoding::Chunked) { | ||||
|                     encodings.push(header::Encoding::Chunked); | ||||
|                 } | ||||
|                 false | ||||
|             }, | ||||
|             None => true | ||||
|         }; | ||||
|  | ||||
|         if encodings { | ||||
|             headers.set(header::TransferEncoding(vec![header::Encoding::Chunked])); | ||||
|         } | ||||
|         Encoder::chunked() | ||||
|     } | ||||
| } | ||||
|  | ||||
| #[derive(Clone, Copy)] | ||||
| struct HeaderIndices { | ||||
|     name: (usize, usize), | ||||
| @@ -421,63 +444,83 @@ mod tests { | ||||
|     fn test_decoder_request() { | ||||
|         use super::Decoder; | ||||
|  | ||||
|         let method = &mut None; | ||||
|         let mut head = MessageHead::<::http::RequestLine>::default(); | ||||
|  | ||||
|         head.subject.0 = ::Method::Get; | ||||
|         assert_eq!(Decoder::length(0), ServerTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(0), ServerTransaction::decoder(&head, method).unwrap()); | ||||
|         assert_eq!(*method, Some(::Method::Get)); | ||||
|  | ||||
|         head.subject.0 = ::Method::Post; | ||||
|         assert_eq!(Decoder::length(0), ServerTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(0), ServerTransaction::decoder(&head, method).unwrap()); | ||||
|         assert_eq!(*method, Some(::Method::Post)); | ||||
|  | ||||
|         head.headers.set(TransferEncoding::chunked()); | ||||
|         assert_eq!(Decoder::chunked(), ServerTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::chunked(), ServerTransaction::decoder(&head, method).unwrap()); | ||||
|         // transfer-encoding and content-length = chunked | ||||
|         head.headers.set(ContentLength(10)); | ||||
|         assert_eq!(Decoder::chunked(), ServerTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::chunked(), ServerTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         head.headers.remove::<TransferEncoding>(); | ||||
|         assert_eq!(Decoder::length(10), ServerTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(10), ServerTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         head.headers.set_raw("Content-Length", vec![b"5".to_vec(), b"5".to_vec()]); | ||||
|         assert_eq!(Decoder::length(5), ServerTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(5), ServerTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         head.headers.set_raw("Content-Length", vec![b"10".to_vec(), b"11".to_vec()]); | ||||
|         ServerTransaction::decoder(&head).unwrap_err(); | ||||
|         ServerTransaction::decoder(&head, method).unwrap_err(); | ||||
|  | ||||
|         head.headers.remove::<ContentLength>(); | ||||
|  | ||||
|         head.headers.set_raw("Transfer-Encoding", "gzip"); | ||||
|         ServerTransaction::decoder(&head).unwrap_err(); | ||||
|         ServerTransaction::decoder(&head, method).unwrap_err(); | ||||
|     } | ||||
|  | ||||
|     #[test] | ||||
|     fn test_decoder_response() { | ||||
|         use super::Decoder; | ||||
|  | ||||
|         let method = &mut Some(::Method::Get); | ||||
|         let mut head = MessageHead::<::http::RawStatus>::default(); | ||||
|  | ||||
|         head.subject.0 = 204; | ||||
|         assert_eq!(Decoder::length(0), ClientTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(0), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|         head.subject.0 = 304; | ||||
|         assert_eq!(Decoder::length(0), ClientTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(0), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         head.subject.0 = 200; | ||||
|         assert_eq!(Decoder::eof(), ClientTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::eof(), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         *method = Some(::Method::Head); | ||||
|         assert_eq!(Decoder::length(0), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         *method = Some(::Method::Connect); | ||||
|         assert_eq!(Decoder::length(0), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|  | ||||
|         // CONNECT receiving non 200 can have a body | ||||
|         head.subject.0 = 404; | ||||
|         head.headers.set(ContentLength(10)); | ||||
|         assert_eq!(Decoder::length(10), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|         head.headers.remove::<ContentLength>(); | ||||
|  | ||||
|  | ||||
|         *method = Some(::Method::Get); | ||||
|         head.headers.set(TransferEncoding::chunked()); | ||||
|         assert_eq!(Decoder::chunked(), ClientTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::chunked(), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         // transfer-encoding and content-length = chunked | ||||
|         head.headers.set(ContentLength(10)); | ||||
|         assert_eq!(Decoder::chunked(), ClientTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::chunked(), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         head.headers.remove::<TransferEncoding>(); | ||||
|         assert_eq!(Decoder::length(10), ClientTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(10), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         head.headers.set_raw("Content-Length", vec![b"5".to_vec(), b"5".to_vec()]); | ||||
|         assert_eq!(Decoder::length(5), ClientTransaction::decoder(&head).unwrap()); | ||||
|         assert_eq!(Decoder::length(5), ClientTransaction::decoder(&head, method).unwrap()); | ||||
|  | ||||
|         head.headers.set_raw("Content-Length", vec![b"10".to_vec(), b"11".to_vec()]); | ||||
|         ClientTransaction::decoder(&head).unwrap_err(); | ||||
|         ClientTransaction::decoder(&head, method).unwrap_err(); | ||||
|     } | ||||
|  | ||||
|     #[cfg(feature = "nightly")] | ||||
| @@ -541,7 +584,7 @@ mod tests { | ||||
|  | ||||
|         b.iter(|| { | ||||
|             let mut vec = Vec::new(); | ||||
|             ServerTransaction::encode(head.clone(), &mut vec); | ||||
|             ServerTransaction::encode(head.clone(), true, &mut None, &mut vec); | ||||
|             assert_eq!(vec.len(), len); | ||||
|             ::test::black_box(vec); | ||||
|         }) | ||||
|   | ||||
| @@ -144,9 +144,8 @@ pub trait Http1Transaction { | ||||
|     type Incoming; | ||||
|     type Outgoing: Default; | ||||
|     fn parse(bytes: &mut BytesMut) -> ParseResult<Self::Incoming>; | ||||
|     fn decoder(head: &MessageHead<Self::Incoming>) -> ::Result<h1::Decoder>; | ||||
|     fn encode(head: MessageHead<Self::Outgoing>, dst: &mut Vec<u8>) -> h1::Encoder; | ||||
|     fn should_set_length(head: &MessageHead<Self::Outgoing>) -> bool; | ||||
|     fn decoder(head: &MessageHead<Self::Incoming>, method: &mut Option<::Method>) -> ::Result<h1::Decoder>; | ||||
|     fn encode(head: MessageHead<Self::Outgoing>, has_body: bool, method: &mut Option<Method>, dst: &mut Vec<u8>) -> h1::Encoder; | ||||
| } | ||||
|  | ||||
| pub type ParseResult<T> = ::Result<Option<(MessageHead<T>, usize)>>; | ||||
|   | ||||
| @@ -1,6 +1,6 @@ | ||||
| #![doc(html_root_url = "https://docs.rs/hyper/0.11.1")] | ||||
| #![deny(missing_docs)] | ||||
| #![deny(warnings)] | ||||
| //#![deny(warnings)] | ||||
| #![deny(missing_debug_implementations)] | ||||
| #![cfg_attr(all(test, feature = "nightly"), feature(test))] | ||||
|  | ||||
|   | ||||
		Reference in New Issue
	
	Block a user