fix(server): [WIP] draft 2427 fixes (update 2!)

there are some tests fixes now, but they may be controversial.

I was moved to take advantage of the fact that Body::empty() provides no
SizeHint to encode().  But, encode would still take a None and write
`content-length: 0`.  For the moment, I've changed that.  But, this may
be an unexpected change for some.

I'd expect most services do not go out of their way to differentiate
between GET and HEAD requests.  This works out fine, in that any
determinably sized body ends up creating a Content-Length header for
either request.

As written in this draft, a 304 response with Body::empty() will have no
Content-Length.  But, if anyone had written one using something like
`"".into()`, this change will now add `content-length: 0`.

Author: jeddenlea

src/body/body.rs

@@ -480,7 +480,7 @@ impl From<Bytes> for Body {
     #[inline]
     fn from(chunk: Bytes) -> Body {
         if chunk.is_empty() {
-            Body::empty()
+            Body::new(Kind::Once(Some(Bytes::new()))) // drop chunk ASAP
         } else {
             Body::new(Kind::Once(Some(chunk)))
         }

src/proto/h1/role.rs

@@ -385,7 +385,7 @@ impl Http1Transaction for Server {
             }};
         }
 
-        'headers: for (opt_name, value) in msg.head.headers.drain() {
+        for (opt_name, value) in msg.head.headers.drain() {
             if let Some(n) = opt_name {
                 cur_name = Some(n);
                 handle_is_name_written!();
@@ -399,6 +399,14 @@ impl Http1Transaction for Server {
                         rewind(dst);
                         return Err(crate::Error::new_user_header());
                     }
+                    // Can this response even contain Content-Length?
+                    if !Server::can_have_body(msg.req_method, msg.head.subject) {
+                        warn!(
+                            "illegal content-length ({:?}) seen for {:?} response to {:?} request",
+                            value, msg.head.subject, msg.req_method
+                        );
+                        continue;
+                    }
                     match msg.body {
                         Some(BodyLength::Known(known_len)) => {
                             // The HttpBody claims to know a length, and
@@ -421,13 +429,15 @@ impl Http1Transaction for Server {
                             }
 
                             if !is_name_written {
-                                encoder = Encoder::length(known_len);
+                                if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                                    encoder = Encoder::length(known_len);
+                                }
                                 extend(dst, b"content-length: ");
                                 extend(dst, value.as_bytes());
                                 wrote_len = true;
                                 is_name_written = true;
                             }
-                            continue 'headers;
+                            continue;
                         }
                         Some(BodyLength::Unknown) => {
                             // The HttpBody impl didn't know how long the
@@ -446,16 +456,18 @@ impl Http1Transaction for Server {
                                         return Err(crate::Error::new_user_header());
                                     }
                                     debug_assert!(is_name_written);
-                                    continue 'headers;
+                                    continue;
                                 } else {
                                     // we haven't written content-length yet!
-                                    encoder = Encoder::length(len);
+                                    if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                                        encoder = Encoder::length(len);
+                                    }
                                     extend(dst, b"content-length: ");
                                     extend(dst, value.as_bytes());
                                     wrote_len = true;
                                     is_name_written = true;
                                     prev_con_len = Some(len);
-                                    continue 'headers;
+                                    continue;
                                 }
                             } else {
                                 warn!("illegal Content-Length value: {:?}", value);
@@ -464,13 +476,13 @@ impl Http1Transaction for Server {
                             }
                         }
                         None => {
-                            // We have no body to actually send,
-                            // but the headers claim a content-length.
-                            // There's only 2 ways this makes sense:
+                            // We have no body to actually send, but the headers claim a
+                            // content-length.  There are 3 ways this makes sense:
                             //
                             // - The header says the length is `0`.
+                            // - The response is NOT_MODIFIED
                             // - This is a response to a `HEAD` request.
-                            if msg.req_method == &Some(Method::HEAD) {
+                            if Server::imply_body_only(msg.req_method, msg.head.subject) {
                                 debug_assert_eq!(encoder, Encoder::length(0));
                             } else {
                                 if value.as_bytes() != b"0" {
@@ -479,7 +491,7 @@ impl Http1Transaction for Server {
                                         value
                                     );
                                 }
-                                continue 'headers;
+                                continue;
                             }
                         }
                     }
@@ -492,9 +504,13 @@ impl Http1Transaction for Server {
                         return Err(crate::Error::new_user_header());
                     }
                     // check that we actually can send a chunked body...
-                    if msg.head.version == Version::HTTP_10
-                        || !Server::can_chunked(msg.req_method, msg.head.subject)
-                    {
+                    if msg.head.version == Version::HTTP_10 {
+                        continue;
+                    } else if !Server::can_have_body(msg.req_method, msg.head.subject) {
+                        warn!(
+                            "illegal transfer-encoding ({:?}) seen for {:?} response to {:?} request",
+                            value, msg.head.subject, msg.req_method
+                        );
                         continue;
                     }
                     wrote_len = true;
@@ -503,15 +519,17 @@ impl Http1Transaction for Server {
                     must_write_chunked = !headers::is_chunked_(&value);
 
                     if !is_name_written {
-                        encoder = Encoder::chunked();
+                        if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                            encoder = Encoder::chunked();
+                        }
                         is_name_written = true;
                         extend(dst, b"transfer-encoding: ");
                         extend(dst, value.as_bytes());
                     } else {
                         extend(dst, b", ");
                         extend(dst, value.as_bytes());
                     }
-                    continue 'headers;
+                    continue;
                 }
                 header::CONNECTION => {
                     if !is_last && headers::connection_close(&value) {
@@ -525,7 +543,7 @@ impl Http1Transaction for Server {
                         extend(dst, b", ");
                         extend(dst, value.as_bytes());
                     }
-                    continue 'headers;
+                    continue;
                 }
                 header::DATE => {
                     wrote_date = true;
@@ -549,44 +567,50 @@ impl Http1Transaction for Server {
 
         handle_is_name_written!();
 
-        if !wrote_len {
-            encoder = match msg.body {
-                Some(BodyLength::Unknown) => {
-                    if msg.head.version == Version::HTTP_10
-                        || !Server::can_chunked(msg.req_method, msg.head.subject)
-                    {
-                        Encoder::close_delimited()
-                    } else {
-                        extend(dst, b"transfer-encoding: chunked\r\n");
-                        Encoder::chunked()
-                    }
+        // We should provide either a Content-Length or Transfer-Encoding header based on what we
+        // know about the Body, unless:
+        //   - the request was HEAD and the body is Body::empty(), because a service may simply not
+        //     provide generate the real body for a HEAD request.
+        //   - the response is a 304, like a HEAD, may imply things about a particular body, but a
+        //     service probably did not provide the full body in its Response.
+        //   - the response must neither contain nor imply a body.
+        if !wrote_len && Server::can_have_body(msg.req_method, msg.head.subject) {
+            match msg.body {
+                Some(BodyLength::Known(0)) => extend(dst, b"content-length: 0\r\n"),
+                Some(BodyLength::Known(len)) => {
+                    extend(dst, b"content-length: ");
+                    let _ = ::itoa::write(&mut dst, len);
+                    extend(dst, b"\r\n");
                 }
-                None | Some(BodyLength::Known(0)) => {
-                    if msg.head.subject != StatusCode::NOT_MODIFIED {
+                None => {
+                    if !Server::imply_body_only(msg.req_method, msg.head.subject) {
                         extend(dst, b"content-length: 0\r\n");
                     }
-                    Encoder::length(0)
                 }
-                Some(BodyLength::Known(len)) => {
-                    if msg.head.subject == StatusCode::NOT_MODIFIED {
-                        Encoder::length(0)
-                    } else {
-                        extend(dst, b"content-length: ");
-                        let _ = ::itoa::write(&mut dst, len);
-                        extend(dst, b"\r\n");
-                        Encoder::length(len)
+                Some(BodyLength::Unknown) => {
+                    if msg.head.version != Version::HTTP_10 {
+                        extend(dst, b"transfer-encoding: chunked\r\n");
                     }
                 }
             };
+
+            if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                encoder = match msg.body {
+                    Some(BodyLength::Unknown) if msg.head.version == Version::HTTP_10 => {
+                        Encoder::close_delimited()
+                    }
+                    Some(BodyLength::Unknown) => Encoder::chunked(),
+                    Some(BodyLength::Known(len)) => Encoder::length(len),
+                    None => Encoder::length(0),
+                };
+            }
         }
 
-        if !Server::can_have_body(msg.req_method, msg.head.subject) {
-            trace!(
-                "server body forced to 0; method={:?}, status={:?}",
-                msg.req_method,
-                msg.head.subject
-            );
-            encoder = Encoder::length(0);
+        #[cfg(debug_assertions)]
+        if Server::imply_body_only(msg.req_method, msg.head.subject)
+            || !Server::can_have_body(msg.req_method, msg.head.subject)
+        {
+            assert_eq!(encoder, Encoder::length(0));
         }
 
         // cached date is much faster than formatting every request
@@ -631,24 +655,19 @@ impl Http1Transaction for Server {
 #[cfg(feature = "server")]
 impl Server {
     fn can_have_body(method: &Option<Method>, status: StatusCode) -> bool {
-        Server::can_chunked(method, status)
-    }
-
-    fn can_chunked(method: &Option<Method>, status: StatusCode) -> bool {
-        if method == &Some(Method::HEAD) || method == &Some(Method::CONNECT) && status.is_success()
+        if method == &Some(Method::CONNECT) && status.is_success()
+            || status == StatusCode::NO_CONTENT
+            || status.is_informational()
         {
             false
         } else {
-            match status {
-                // TODO: support for 1xx codes needs improvement everywhere
-                // would be 100...199 => false
-                StatusCode::SWITCHING_PROTOCOLS
-                | StatusCode::NO_CONTENT
-                | StatusCode::NOT_MODIFIED => false,
-                _ => true,
-            }
+            true
         }
     }
+
+    fn imply_body_only(method: &Option<Method>, status: StatusCode) -> bool {
+        method == &Some(Method::HEAD) || status == StatusCode::NOT_MODIFIED
+    }
 }
 
 #[cfg(feature = "client")]

tests/client.rs

@@ -359,6 +359,25 @@ test! {
         expected: "GET / HTTP/1.1\r\nhost: {addr}\r\n\r\n",
         reply: REPLY_OK,
 
+    client:
+        request: {
+            method: GET,
+            url: "http://{addr}/",
+            // body is Body::empty
+        },
+        response:
+            status: OK,
+            headers: {},
+            body: None,
+}
+
+test! {
+    name: client_get_req_body_explicitly_empty,
+
+    server:
+        expected: "GET / HTTP/1.1\r\nhost: {addr}\r\ncontent-length: 0\r\n\r\n",
+        reply: REPLY_OK,
+
     client:
         request: {
             method: GET,

tests/server.rs

@@ -515,7 +515,7 @@ fn response_does_not_set_chunked_if_body_not_allowed() {
     let server = serve();
     server
         .reply()
-        .status(hyper::StatusCode::NOT_MODIFIED)
+        .status(hyper::StatusCode::NO_CONTENT)
         .header("transfer-encoding", "chunked");
     let mut req = connect(server.addr());
     req.write_all(
@@ -534,7 +534,7 @@ fn response_does_not_set_chunked_if_body_not_allowed() {
     assert!(!response.contains("transfer-encoding"));
 
     let mut lines = response.lines();
-    assert_eq!(lines.next(), Some("HTTP/1.1 304 Not Modified"));
+    assert_eq!(lines.next(), Some("HTTP/1.1 204 No Content"));
 
     // no body or 0\r\n\r\n
     let mut lines = lines.skip_while(|line| !line.is_empty());
@@ -1775,7 +1775,7 @@ async fn http2_service_poll_ready_error_sends_goaway() {
 }
 
 #[test]
-fn skips_content_length_for_304_responses() {
+fn keeps_content_length_and_drops_body_for_304_responses() {
     let server = serve();
     server
         .reply()
@@ -1794,15 +1794,21 @@ fn skips_content_length_for_304_responses() {
 
     let mut response = String::new();
     req.read_to_string(&mut response).unwrap();
-    assert!(!response.contains("content-length:"));
+    assert!(response.contains("content-length: 3"));
+    let mut lines = response.lines();
+    assert_eq!(lines.next(), Some("HTTP/1.1 304 Not Modified"));
+
+    let mut lines = lines.skip_while(|line| !line.is_empty());
+    assert_eq!(lines.next(), Some(""));
+    assert_eq!(lines.next(), None);
 }
 
 #[test]
-fn skips_content_length_and_body_for_304_responses() {
+fn skips_content_length_and_body_for_204_response() {
     let server = serve();
     server
         .reply()
-        .status(hyper::StatusCode::NOT_MODIFIED)
+        .status(hyper::StatusCode::NO_CONTENT)
         .body("foo");
     let mut req = connect(server.addr());
     req.write_all(
@@ -1819,7 +1825,7 @@ fn skips_content_length_and_body_for_304_responses() {
     req.read_to_string(&mut response).unwrap();
     assert!(!response.contains("content-length:"));
     let mut lines = response.lines();
-    assert_eq!(lines.next(), Some("HTTP/1.1 304 Not Modified"));
+    assert_eq!(lines.next(), Some("HTTP/1.1 204 No Content"));
 
     let mut lines = lines.skip_while(|line| !line.is_empty());
     assert_eq!(lines.next(), Some(""));