Repository: netty/netty
Issue: Automatically adding chunked encoding header breaks streaming 
Labels: defect

Previously, a change was committed to automatically add the Transfer-Encoding: chunk header to any HttpMessage for which isChunked() == true.
https://github.com/netty/netty/issues/171
https://github.com/netty/netty/commit/01225b0a522720a7423390673581a0e48bf1d7c2

According to the javadoc
https://github.com/netty/netty/blob/3/src/main/java/org/jboss/netty/handler/codec/http/HttpMessage.java#L133
isChunked isn't coupled to the transfer-encoding.  It can also be used for streaming request/responses.

"This means that the HttpMessageEncoder will add the chunked Transfer-Encoding header to the message and prepend each chunk with <chunksize>CRLF as it is encoded, even for messages that include a Content-Length header for streaming."
This results in a larger than expected message due to the added bytes for the per-chunk size information.
http://en.wikipedia.org/wiki/Chunked_transfer_encoding#Format
which means that clients will think that they have the entire message when it has actually been corrupted by the inserted chunked encoding headers.

The HTTP 1.1 spec specifies that no response should contain both a content-length header and a non-identity transfer-encoding header.
http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4

While it is true that the HTTP 1.1 spec states that responses containing both content-length and transfer-encoding: chunked should be handled by ignoring the content-length field
we have found that some clients do not respect this.


Issue comments:
The fix would be modifying `HttpMessageEncoder` so that it removes `Content-Length` when `Transfer-Encoding: chunked` is set.  WDYT?

@trustin makes sense.. should I make the change later today ?

If you wish to do it by yourself, please go ahead. :-)

On Thu, Aug 9, 2012 at 5:39 PM, Norman Maurer notifications@github.comwrote:

> @trustin https://github.com/trustin makes sense.. should I make the
> change later today ?
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/netty/netty/issues/494#issuecomment-7608522.

Why not avoid adding the transfer-encoding header if content-length is already set?
That way you avoid sending the extra chunked encoding headers along with the message and send the message as it was intended.

Preserving the Content-Length header rather than transforming to chunked encoding also allows the client to show a determinate download progress bar and immediately reject content that is too large.

Agreed.  So, the new behavior should be: if Content-Length is set,
Transfer-Encoding header is cleared and HttpMessageEncoder.chunked is set
to false.  Does that make sense?

On Fri, Aug 10, 2012 at 1:43 AM, Michael Zampani
notifications@github.comwrote:

> Preserving the Content-Length header rather than transforming to chunked
> encoding also allows the client to show a determinate download progress bar
> and immediately reject content that is too large.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/netty/netty/issues/494#issuecomment-7619551.

@zampani what about issue a pull request ?

@zampani Would you mind if I ask you to confirm the fix is working as you expected?


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/handler/codec/http/HttpCodecUtil.java b/src/main/java/org/jboss/netty/handler/codec/http/HttpCodecUtil.java
index 94c685f6f5b..8105f67fd96 100644
--- a/src/main/java/org/jboss/netty/handler/codec/http/HttpCodecUtil.java
+++ b/src/main/java/org/jboss/netty/handler/codec/http/HttpCodecUtil.java
@@ -118,6 +118,17 @@ static boolean isTransferEncodingChunked(HttpMessage m) {
         return false;
     }
 
+    static void removeTransferEncodingChunked(HttpMessage m) {
+        List<String> values = m.getHeaders(HttpHeaders.Names.TRANSFER_ENCODING);
+        values.remove(HttpHeaders.Values.CHUNKED);
+        m.setHeader(HttpHeaders.Names.TRANSFER_ENCODING, values);
+    }
+
+    static boolean isContentLengthSet(HttpMessage m) {
+        List<String> contentLength = m.getHeaders(HttpHeaders.Names.CONTENT_LENGTH);
+        return !contentLength.isEmpty();
+    }
+
     private HttpCodecUtil() {
         super();
     }
diff --git a/src/main/java/org/jboss/netty/handler/codec/http/HttpMessageEncoder.java b/src/main/java/org/jboss/netty/handler/codec/http/HttpMessageEncoder.java
index 0a29aaae4f8..6dcb3694bfa 100644
--- a/src/main/java/org/jboss/netty/handler/codec/http/HttpMessageEncoder.java
+++ b/src/main/java/org/jboss/netty/handler/codec/http/HttpMessageEncoder.java
@@ -65,12 +65,18 @@ protected Object encode(ChannelHandlerContext ctx, Channel channel, Object msg)
             HttpMessage m = (HttpMessage) msg;
             boolean chunked;
             if (m.isChunked()) {
-                // check if the Transfer-Encoding is set to chunked already.
-                // if not add the header to the message
-                if (!HttpCodecUtil.isTransferEncodingChunked(m)) {
-                    m.addHeader(Names.TRANSFER_ENCODING, Values.CHUNKED);
+                // if Content-Length is set then the message can't be HTTP chunked
+                if (HttpCodecUtil.isContentLengthSet(m)) {
+                    chunked = this.chunked = false;
+                    HttpCodecUtil.removeTransferEncodingChunked(m);
+                } else {
+                    // check if the Transfer-Encoding is set to chunked already.
+                    // if not add the header to the message
+                    if (!HttpCodecUtil.isTransferEncodingChunked(m)) {
+                        m.addHeader(Names.TRANSFER_ENCODING, Values.CHUNKED);
+                    }
+                    chunked = this.chunked = true;
                 }
-                chunked = this.chunked = true;
             } else {
                 chunked = this.chunked = HttpCodecUtil.isTransferEncodingChunked(m);
             }


Test patch: