From 41b0080fcc8fcace7cd62d238f6a932e79ec8bb1 Mon Sep 17 00:00:00 2001
From: Roelof Naude <roelof.naude@gmail.com>
Date: Thu, 7 May 2015 08:53:34 +0200
Subject: [PATCH] Support empty http responses when using compression

Motivation:

Found a bug in that netty would generate a 20 byte body when returing a response
to an HTTP HEAD. the 20 bytes seems to be related to the compression footer.

RFC2616, section 9.4 states that responses to an HTTP HEAD MUST not return a message
body in the response.

Netty's own client implementation expected an empty response. The extra bytes lead to a
2nd response with an error decoder result:
java.lang.IllegalArgumentException: invalid version format: 14

Modifications:

Track the HTTP request method. When processing the response we determine if the response
is passthru unnchanged. This decision now takes into account the request method and passthru
responses related to HTTP HEAD requests.

Result:

Netty's http client works and better RFC conformance.
---
 .../codec/http/HttpContentEncoder.java        | 44 ++++++---
 .../codec/http/HttpContentEncoderTest.java    | 90 +++++++++++++++++++
 2 files changed, 121 insertions(+), 13 deletions(-)

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 52bf5f1f3a..111621bb52 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
@@ -58,8 +58,11 @@ public abstract class HttpContentEncoder extends MessageToMessageCodec<HttpReque
         AWAIT_CONTENT
     }
 
-    private final Queue<String> acceptEncodingQueue = new ArrayDeque<String>();
-    private String acceptEncoding;
+    private static final CharSequence ZERO_LENGTH_HEAD = "HEAD";
+    private static final CharSequence ZERO_LENGTH_CONNECT = "CONNECT";
+
+    private final Queue<CharSequence> acceptEncodingQueue = new ArrayDeque<CharSequence>();
+    private CharSequence acceptEncoding;
     private EmbeddedChannel encoder;
     private State state = State.AWAIT_HEADERS;
 
@@ -71,10 +74,18 @@ public abstract class HttpContentEncoder extends MessageToMessageCodec<HttpReque
     @Override
     protected void decode(ChannelHandlerContext ctx, HttpRequest msg, List<Object> out)
             throws Exception {
-        String acceptedEncoding = msg.headers().get(HttpHeaders.Names.ACCEPT_ENCODING);
+        CharSequence acceptedEncoding = msg.headers().get(HttpHeaders.Names.ACCEPT_ENCODING);
         if (acceptedEncoding == null) {
             acceptedEncoding = HttpHeaders.Values.IDENTITY;
         }
+
+        HttpMethod meth = msg.getMethod();
+        if (meth == HttpMethod.HEAD) {
+            acceptedEncoding = ZERO_LENGTH_HEAD;
+        } else if (meth == HttpMethod.CONNECT) {
+            acceptedEncoding = ZERO_LENGTH_CONNECT;
+        }
+
         acceptEncodingQueue.add(acceptedEncoding);
         out.add(ReferenceCountUtil.retain(msg));
     }
@@ -89,12 +100,24 @@ public abstract class HttpContentEncoder extends MessageToMessageCodec<HttpReque
 
                 final HttpResponse res = (HttpResponse) msg;
 
+                // Get the list of encodings accepted by the peer.
+                acceptEncoding = acceptEncodingQueue.poll();
+                if (acceptEncoding == null) {
+                    throw new IllegalStateException("cannot send more responses than requests");
+                }
+
                 /*
                  * per rfc2616 4.3 Message Body
                  * All 1xx (informational), 204 (no content), and 304 (not modified) responses MUST NOT include a
                  * message-body. All other responses do include a message-body, although it MAY be of zero length.
+                 *
+                 * 9.4 HEAD
+                 * 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
                  */
-                if (isPassthru(res)) {
+                if (isPassthru(res, acceptEncoding)) {
                     if (isFull) {
                         out.add(ReferenceCountUtil.retain(res));
                     } else {
@@ -105,12 +128,6 @@ public abstract class HttpContentEncoder extends MessageToMessageCodec<HttpReque
                     break;
                 }
 
-                // Get the list of encodings accepted by the peer.
-                acceptEncoding = acceptEncodingQueue.poll();
-                if (acceptEncoding == null) {
-                    throw new IllegalStateException("cannot send more responses than requests");
-                }
-
                 if (isFull) {
                     // Pass through the full response with empty content and continue waiting for the the next resp.
                     if (!((ByteBufHolder) res).content().isReadable()) {
@@ -120,7 +137,7 @@ public abstract class HttpContentEncoder extends MessageToMessageCodec<HttpReque
                 }
 
                 // Prepare to encode the content.
-                final Result result = beginEncode(res, acceptEncoding);
+                final Result result = beginEncode(res, acceptEncoding.toString());
 
                 // If unable to encode, pass through.
                 if (result == null) {
@@ -181,9 +198,10 @@ public abstract class HttpContentEncoder extends MessageToMessageCodec<HttpReque
         }
     }
 
-    private static boolean isPassthru(HttpResponse res) {
+    private static boolean isPassthru(HttpResponse res, CharSequence httpMethod) {
         final int code = res.getStatus().code();
-        return code < 200 || code == 204 || code == 304;
+        boolean expectEmptyBody = httpMethod == ZERO_LENGTH_HEAD || (httpMethod == ZERO_LENGTH_CONNECT && code == 200);
+        return code < 200 || code == 204 || code == 304 || expectEmptyBody;
     }
 
     private static void ensureHeaders(HttpObject msg) {
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 094e6080a7..e0431f043c 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
@@ -254,6 +254,96 @@ public class HttpContentEncoderTest {
         assertThat(ch.readOutbound(), is(nullValue()));
     }
 
+    @Test
+    public void testEmptyHeadResponse() throws Exception {
+        EmbeddedChannel ch = new EmbeddedChannel(new TestEncoder());
+        HttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.HEAD, "/");
+        ch.writeInbound(req);
+
+        HttpResponse res = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
+        res.headers().set(Names.TRANSFER_ENCODING, Values.CHUNKED);
+        ch.writeOutbound(res);
+        ch.writeOutbound(LastHttpContent.EMPTY_LAST_CONTENT);
+
+        assertEmptyResponse(ch);
+    }
+
+    @Test
+    public void testHttp304Response() throws Exception {
+        EmbeddedChannel ch = new EmbeddedChannel(new TestEncoder());
+        HttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/");
+        req.headers().set(Names.ACCEPT_ENCODING, Values.GZIP);
+        ch.writeInbound(req);
+
+        HttpResponse res = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.NOT_MODIFIED);
+        res.headers().set(Names.TRANSFER_ENCODING, Values.CHUNKED);
+        ch.writeOutbound(res);
+        ch.writeOutbound(LastHttpContent.EMPTY_LAST_CONTENT);
+
+        assertEmptyResponse(ch);
+    }
+
+    @Test
+    public void testConnect200Response() throws Exception {
+        EmbeddedChannel ch = new EmbeddedChannel(new TestEncoder());
+        HttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.CONNECT, "google.com:80");
+        ch.writeInbound(req);
+
+        HttpResponse res = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
+        res.headers().set(Names.TRANSFER_ENCODING, Values.CHUNKED);
+        ch.writeOutbound(res);
+        ch.writeOutbound(LastHttpContent.EMPTY_LAST_CONTENT);
+
+        assertEmptyResponse(ch);
+    }
+
+    @Test
+    public void testConnectFailureResponse() throws Exception {
+        String content = "Not allowed by configuration";
+
+        EmbeddedChannel ch = new EmbeddedChannel(new TestEncoder());
+        HttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.CONNECT, "google.com:80");
+        ch.writeInbound(req);
+
+        HttpResponse res = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.METHOD_NOT_ALLOWED);
+        res.headers().set(Names.TRANSFER_ENCODING, Values.CHUNKED);
+        ch.writeOutbound(res);
+        ch.writeOutbound(new DefaultHttpContent(Unpooled.wrappedBuffer(content.getBytes(CharsetUtil.UTF_8))));
+        ch.writeOutbound(LastHttpContent.EMPTY_LAST_CONTENT);
+
+        assertEncodedResponse(ch);
+        Object o = ch.readOutbound();
+        assertThat(o, is(instanceOf(HttpContent.class)));
+        HttpContent chunk = (HttpContent) o;
+        assertThat(chunk.content().toString(CharsetUtil.US_ASCII), is("28"));
+        chunk.release();
+
+        chunk = (HttpContent) ch.readOutbound();
+        assertThat(chunk.content().isReadable(), is(true));
+        assertThat(chunk.content().toString(CharsetUtil.US_ASCII), is("0"));
+        chunk.release();
+
+        chunk = (HttpContent) ch.readOutbound();
+        assertThat(chunk, is(instanceOf(LastHttpContent.class)));
+        chunk.release();
+        assertThat(ch.readOutbound(), is(nullValue()));
+    }
+
+    private static void assertEmptyResponse(EmbeddedChannel ch) {
+        Object o = ch.readOutbound();
+        assertThat(o, is(instanceOf(HttpResponse.class)));
+
+        HttpResponse res = (HttpResponse) o;
+        assertThat(res, is(not(instanceOf(HttpContent.class))));
+        assertThat(res.headers().get(Names.TRANSFER_ENCODING), is("chunked"));
+        assertThat(res.headers().get(Names.CONTENT_LENGTH), is(nullValue()));
+
+        HttpContent chunk = (HttpContent) ch.readOutbound();
+        assertThat(chunk, is(instanceOf(LastHttpContent.class)));
+        chunk.release();
+        assertThat(ch.readOutbound(), is(nullValue()));
+    }
+
     private static void assertEncodedResponse(EmbeddedChannel ch) {
         Object o = ch.readOutbound();
         assertThat(o, is(instanceOf(HttpResponse.class)));