Repository: netty/netty
Issue: HttpContentCompressor/HttpContentEncoder should not set chunked transfer-encoding for HTTP 1.0
Labels: defect

Netty version: 4.1.0.Final

Context:
When using HttpContentCompressor and the HttpResponse is protocol version 1.0, HttpContentEncoder.encode() should not set the transfer-encoding header to chunked.  Chunked transfer-encoding is not valid for HTTP 1.0 - this causes ERR_CONTENT_DECODING_FAILED errors in chrome and similar failures in IE.

This is the snippet I'm referring to:

// Make the response chunked to simplify content transformation.
res.headers().remove(HttpHeaderNames.CONTENT_LENGTH);
res.headers().set(HttpHeaderNames.TRANSFER_ENCODING, HttpHeaderValues.CHUNKED);


Issue comments:
@mrokitka wow there are still people using HTTP/1.0 ? You are right we are currently only support it with HTTP/1.1, not sure if we will ever support HTTP/1.0. But at least we should "skip" HTTP/1.0 messages. Let me come up with a PR for this

@normanmaurer - I've run into this since we're using Squid as a caching proxy to handle caching of content for outbound proxy requests from a Netty based proxy server I'm implementing.  Squid doesn't fully support HTTP 1.1, so although it handles HTTP compression properly responses are always HTTP 1.0.

Kind of a drag, but if the compressor could handle the compression for HTTP 1.0 without the need for chunked encoding that would be great.

@mrokitka the problem with this is that we would need buffer the full content in memory as we will need to set the "Content-Length" header. We could do this but not sure if this is really good :)

@normanmaurer perhaps a flag that could be programmatically set on the compressor to indicate if we want to support HTTP 1.0?  A true value would compress and buffer to set the content-length header for 1.0 responses, false would just skip 1.0 messages?

@mrokitka maybe... let me see what I can come up with.

Fixed by https://github.com/netty/netty/pull/5405 . 

@mrokitka I may come up with a follow up to support compression for http 1.0 as well but currently very busy. If you like you can also contribute a PR.

https://github.com/netty/netty/pull/5405


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
index a20a3fd7897..ec1856673c1 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
@@ -120,9 +120,11 @@ protected void encode(ChannelHandlerContext ctx, HttpObject msg, List<Object> ou
                  * The HEAD method is identical to GET except that the server MUST NOT return a message-body
                  * in the response.
                  *
-                 * This code is now inline with HttpClientDecoder.Decoder
+                 * Also we should pass through HTTP/1.0 as transfer-encoding: chunked is not supported.
+                 *
+                 * See https://github.com/netty/netty/issues/5382
                  */
-                if (isPassthru(code, acceptEncoding)) {
+                if (isPassthru(res.protocolVersion(), code, acceptEncoding)) {
                     if (isFull) {
                         out.add(ReferenceCountUtil.retain(res));
                     } else {
@@ -203,9 +205,10 @@ protected void encode(ChannelHandlerContext ctx, HttpObject msg, List<Object> ou
         }
     }
 
-    private static boolean isPassthru(int code, CharSequence httpMethod) {
+    private static boolean isPassthru(HttpVersion version, int code, CharSequence httpMethod) {
         return code < 200 || code == 204 || code == 304 ||
-               (httpMethod == ZERO_LENGTH_HEAD || (httpMethod == ZERO_LENGTH_CONNECT && code == 200));
+               (httpMethod == ZERO_LENGTH_HEAD || (httpMethod == ZERO_LENGTH_CONNECT && code == 200)) ||
+                version == HttpVersion.HTTP_1_0;
     }
 
     private static void ensureHeaders(HttpObject msg) {


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentEncoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentEncoderTest.java
index dd774267564..660df3406e5 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentEncoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentEncoderTest.java
@@ -30,7 +30,10 @@
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.nullValue;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 
 public class HttpContentEncoderTest {
 
@@ -332,6 +335,31 @@ public void testConnectFailureResponse() throws Exception {
         assertThat(ch.readOutbound(), is(nullValue()));
     }
 
+    @Test
+    public void testHttp1_0() throws Exception {
+        EmbeddedChannel ch = new EmbeddedChannel(new TestEncoder());
+        FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_0, HttpMethod.GET, "/");
+        assertTrue(ch.writeInbound(req));
+
+        HttpResponse res = new DefaultHttpResponse(HttpVersion.HTTP_1_0, HttpResponseStatus.OK);
+        res.headers().set(HttpHeaderNames.CONTENT_LENGTH, HttpHeaderValues.ZERO);
+        assertTrue(ch.writeOutbound(res));
+        assertTrue(ch.writeOutbound(LastHttpContent.EMPTY_LAST_CONTENT));
+        assertTrue(ch.finish());
+
+        FullHttpRequest request = ch.readInbound();
+        assertTrue(request.release());
+        assertNull(ch.readInbound());
+
+        HttpResponse response = ch.readOutbound();
+        assertSame(res, response);
+
+        LastHttpContent content = ch.readOutbound();
+        assertSame(LastHttpContent.EMPTY_LAST_CONTENT, content);
+        content.release();
+        assertNull(ch.readOutbound());
+    }
+
     private static void assertEmptyResponse(EmbeddedChannel ch) {
         Object o = ch.readOutbound();
         assertThat(o, is(instanceOf(HttpResponse.class)));