Repository: netty/netty
Issue: HttpServerCodec annot encode a respons e to HEAD request with a 'content-encoding: chunked' header
Labels: defect

Netty versions: 4.x
### Context

It is valid to send a response to a HEAD request that contains a `transfer-encoding: chunked` header, but there is no way to do this using the netty4 `HttpServerCodec`.
### Root of the problem

The root cause is that the netty4 `HttpObjectEncoder` will [transition to the state](https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java#L80) `ST_CONTENT_CHUNK` and the only way to transition back to `ST_INIT` is through the `encodeChunkedContent` method which [will write the terminating length](https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java#L163-L180) (`0\r\n\r\n\r\n`), a protocol error when responding to a HEAD request. I don't believe this is a problem with netty3: it looks like submitting a new `HttpMessage` [effectively resets the encoder](https://github.com/netty/netty/blob/3.10/src/main/java/org/jboss/netty/handler/codec/http/HttpMessageEncoder.java#L62-L101), while the same strategy with netty4 [results in an illegal state exception](https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java#L68-L70).
### Workaround

The `content-encoding` MAY be omitted ([RFC 7231](https://tools.ietf.org/html/rfc7231#section-4.3.2)), but it would be nice to have.


Issue comments:
@bryce-anderson interested to provide a PR + unit test ?

@normanmaurer Do you mean PR to fix or PR to demonstrate? I'm happy to provide a PR either way, I just want to be clear on what PR we want. 😄

If we are talking about a fix, the easy solution is to adopt the netty3 behavior of resetting the encoder on every new `HttpMessage`, but before making the change I think it's worth quickly discussing if that is an acceptable solution: in my minds eye someone introduced that `IllegalStateException` for a reason.

let me fix this...

@bryce-anderson actually I not understand how this happens. Can you provide a unit test that shows it ?

@bryce-anderson nevermind... I think I understand it now

Ah, and I just hacked together an example! Just in case, here it is: https://github.com/bryce-anderson/netty/commit/0fdfcacb67eff649699d266ea6425cb2b93b1218

I apologize in advance, there is probably a cleaner way to do it.

@bryce-anderson PTAL at https://github.com/netty/netty/pull/5846

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

Pull request review comments:
move inner `if` to this level to make an `} else if { ... } else { ... }`

done

Could we define an overridable protected method in `HttpObjectEncoder` rather than duplicating HEAD check in `encode*Content()` methods? That way, `HttpObjectEncoder` could keep `encode*Content()` private. i.e.

``` java
protected boolean isContentAlwaysEmpty(HttpMessage msg) { // subclasses will override.
    return false;
}
```

Actually, this is what we are doing for `HttpObjectDecoder`, so it's probably good to be consistent?

@trustin actually I think this will not work as we need to map request  to response which we only want to do in the `HttpServerCodec`. Or I am missing something ?

@trustin ping

@trustin ping again... sorry for being a PITA.
@Scottmitch as @trustin seems to be busy wdyt ? I think what @trustin suggested can't be done so I think we should pull it in as it is.
@normanmaurer - I think what @trustin was suggesting was to do something like this:


```java
class HttpObjectEncoder {
  final boolean encodeNonChunkedContent(ByteBuf buf, Object msg, long contentLength, List<Object> out) {
  // use isContentAlwaysEmpty, release the msg && add empty buff if true
  }

  final boolean encodeChunkedContent(ChannelHandlerContext ctx, Object msg, long contentLength, List<Object> out) {
  // use isContentAlwaysEmpty, release the msg && add empty buff if true
  }

  protected boolean isContentAlwaysEmpty(HttpMessage msg) {
    return false;
  }
}

// In HttpServerCodec.java
private final class HttpServerResponseEncoder extends HttpResponseEncoder {
  @Override
  protected boolean isContentAlwaysEmpty(HttpMessage msg) {
    if (msg instanceof LastHttpContent) {
      HttpMethod method = queue.poll();
      return HttpMethod.HEAD.equals(method);
    }
    return false;
  }
}
```
@Scottmitch  Ah got it!  Let me update the PR, this is a good idea.
![MAJOR](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-major.png) Remove this unused method parameter "msg". [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=squid%3AS1172)

we have a bunch of conditionals on `state` ... can we make this a `switch`?
the `List` interface allows base classes to do some interesting things which may make this iteration not do exactly what we want. Maybe its OK to assume the super class behavior only adds to the list, and doesn't change indexes though bcz its an internal class? This method is also used in HttpClientCodec too. We have discussed before but it seems like a good idea to remove ambiguity and overhead by removing the the list interface (for future releases).
add a call to `assertFalse(ch.finishAndReleaseAll());` or something similar to assert there is no more data and we clean everything up.
yep for future releases, for now its ok.
+1

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
index dba6f01de2e..10794983fdc 100755
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
@@ -57,6 +57,7 @@ public abstract class HttpObjectEncoder<H extends HttpMessage> extends MessageTo
     private static final int ST_INIT = 0;
     private static final int ST_CONTENT_NON_CHUNK = 1;
     private static final int ST_CONTENT_CHUNK = 2;
+    private static final int ST_CONTENT_ALWAYS_EMPTY = 3;
 
     @SuppressWarnings("RedundantFieldInitialization")
     private int state = ST_INIT;
@@ -77,7 +78,8 @@ protected void encode(ChannelHandlerContext ctx, Object msg, List<Object> out) t
             encodeInitialLine(buf, m);
             encodeHeaders(m.headers(), buf);
             buf.writeBytes(CRLF);
-            state = HttpUtil.isTransferEncodingChunked(m) ? ST_CONTENT_CHUNK : ST_CONTENT_NON_CHUNK;
+            state = isContentAlwaysEmpty(m) ? ST_CONTENT_ALWAYS_EMPTY :
+                    HttpUtil.isTransferEncodingChunked(m) ? ST_CONTENT_CHUNK : ST_CONTENT_NON_CHUNK;
         }
 
         // Bypass the encoder in case of an empty buffer, so that the following idiom works:
@@ -92,44 +94,50 @@ protected void encode(ChannelHandlerContext ctx, Object msg, List<Object> out) t
         }
 
         if (msg instanceof HttpContent || msg instanceof ByteBuf || msg instanceof FileRegion) {
-
-            if (state == ST_INIT) {
-                throw new IllegalStateException("unexpected message type: " + StringUtil.simpleClassName(msg));
-            }
-
-            final long contentLength = contentLength(msg);
-            if (state == ST_CONTENT_NON_CHUNK) {
-                if (contentLength > 0) {
-                    if (buf != null && buf.writableBytes() >= contentLength && msg instanceof HttpContent) {
-                        // merge into other buffer for performance reasons
-                        buf.writeBytes(((HttpContent) msg).content());
-                        out.add(buf);
+            switch (state) {
+                case ST_INIT:
+                    throw new IllegalStateException("unexpected message type: " + StringUtil.simpleClassName(msg));
+                case ST_CONTENT_ALWAYS_EMPTY:
+                    out.add(EMPTY_BUFFER);
+                    if (msg instanceof LastHttpContent) {
+                        state = ST_INIT;
+                    }
+                    return;
+                case ST_CONTENT_NON_CHUNK:
+                    final long contentLength = contentLength(msg);
+                    if (contentLength > 0) {
+                        if (buf != null && buf.writableBytes() >= contentLength && msg instanceof HttpContent) {
+                            // merge into other buffer for performance reasons
+                            buf.writeBytes(((HttpContent) msg).content());
+                            out.add(buf);
+                        } else {
+                            if (buf != null) {
+                                out.add(buf);
+                            }
+                            out.add(encodeAndRetain(msg));
+                        }
                     } else {
                         if (buf != null) {
                             out.add(buf);
+                        } else {
+                            // Need to produce some output otherwise an
+                            // IllegalStateException will be thrown
+                            out.add(EMPTY_BUFFER);
                         }
-                        out.add(encodeAndRetain(msg));
                     }
-                } else {
+
+                    if (msg instanceof LastHttpContent) {
+                        state = ST_INIT;
+                    }
+                    return;
+                case ST_CONTENT_CHUNK:
                     if (buf != null) {
                         out.add(buf);
-                    } else {
-                        // Need to produce some output otherwise an
-                        // IllegalStateException will be thrown
-                        out.add(EMPTY_BUFFER);
                     }
-                }
-
-                if (msg instanceof LastHttpContent) {
-                    state = ST_INIT;
-                }
-            } else if (state == ST_CONTENT_CHUNK) {
-                if (buf != null) {
-                    out.add(buf);
-                }
-                encodeChunkedContent(ctx, msg, contentLength, out);
-            } else {
-                throw new Error();
+                    encodeChunkedContent(ctx, msg, contentLength(msg), out);
+                    return;
+                default:
+                    throw new Error();
             }
         } else {
             if (buf != null) {
@@ -187,6 +195,10 @@ private void encodeChunkedContent(ChannelHandlerContext ctx, Object msg, long co
         }
     }
 
+    boolean isContentAlwaysEmpty(@SuppressWarnings("unused") H msg) {
+        return false;
+    }
+
     @Override
     public boolean acceptOutboundMessage(Object msg) throws Exception {
         return msg instanceof HttpObject || msg instanceof ByteBuf || msg instanceof FileRegion;
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java
index 684af0b67a0..29a11992ad5 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java
@@ -15,9 +15,14 @@
  */
 package io.netty.handler.codec.http;
 
+import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.CombinedChannelDuplexHandler;
 
+import java.util.ArrayDeque;
+import java.util.List;
+import java.util.Queue;
+
 /**
  * A combination of {@link HttpRequestDecoder} and {@link HttpResponseEncoder}
  * which enables easier server side HTTP implementation.
@@ -27,6 +32,9 @@
 public final class HttpServerCodec extends CombinedChannelDuplexHandler<HttpRequestDecoder, HttpResponseEncoder>
         implements HttpServerUpgradeHandler.SourceCodec {
 
+    /** A queue that is used for correlating a request and a response. */
+    private final Queue<HttpMethod> queue = new ArrayDeque<HttpMethod>();
+
     /**
      * Creates a new instance with the default decoder options
      * ({@code maxInitialLineLength (4096}}, {@code maxHeaderSize (8192)}, and
@@ -40,15 +48,16 @@ public HttpServerCodec() {
      * Creates a new instance with the specified decoder options.
      */
     public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize) {
-        super(new HttpRequestDecoder(maxInitialLineLength, maxHeaderSize, maxChunkSize), new HttpResponseEncoder());
+        init(new HttpServerRequestDecoder(maxInitialLineLength, maxHeaderSize, maxChunkSize),
+                new HttpServerResponseEncoder());
     }
 
     /**
      * Creates a new instance with the specified decoder options.
      */
     public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean validateHeaders) {
-        super(new HttpRequestDecoder(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders),
-                new HttpResponseEncoder());
+        init(new HttpServerRequestDecoder(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders),
+                new HttpServerResponseEncoder());
     }
 
     /**
@@ -56,9 +65,10 @@ public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunk
      */
     public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean validateHeaders,
                            int initialBufferSize) {
-        super(
-          new HttpRequestDecoder(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize),
-          new HttpResponseEncoder());
+        init(
+          new HttpServerRequestDecoder(maxInitialLineLength, maxHeaderSize, maxChunkSize,
+                  validateHeaders, initialBufferSize),
+          new HttpServerResponseEncoder());
     }
 
     /**
@@ -69,4 +79,41 @@ public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunk
     public void upgradeFrom(ChannelHandlerContext ctx) {
         ctx.pipeline().remove(this);
     }
+
+    private final class HttpServerRequestDecoder extends HttpRequestDecoder {
+        public HttpServerRequestDecoder(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize) {
+            super(maxInitialLineLength, maxHeaderSize, maxChunkSize);
+        }
+
+        public HttpServerRequestDecoder(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize,
+                                        boolean validateHeaders) {
+            super(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders);
+        }
+
+        public HttpServerRequestDecoder(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize,
+                                        boolean validateHeaders, int initialBufferSize) {
+            super(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize);
+        }
+
+        @Override
+        protected void decode(ChannelHandlerContext ctx, ByteBuf buffer, List<Object> out) throws Exception {
+            int oldSize = out.size();
+            super.decode(ctx, buffer, out);
+            int size = out.size();
+            for (int i = oldSize; i < size; i++) {
+                Object obj = out.get(i);
+                if (obj instanceof HttpRequest) {
+                    queue.add(((HttpRequest) obj).method());
+                }
+            }
+        }
+    }
+
+    private final class HttpServerResponseEncoder extends HttpResponseEncoder {
+
+        @Override
+        boolean isContentAlwaysEmpty(@SuppressWarnings("unused") HttpResponse msg) {
+            return HttpMethod.HEAD.equals(queue.poll());
+        }
+    }
 }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerCodecTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerCodecTest.java
index 345e9e821fc..30878198947 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerCodecTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerCodecTest.java
@@ -114,6 +114,37 @@ public void test100Continue() throws Exception {
         ch.finish();
     }
 
+    @Test
+    public void testChunkedHeadResponse() {
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpServerCodec());
+
+        // Send the request headers.
+        assertTrue(ch.writeInbound(Unpooled.copiedBuffer(
+                "HEAD / HTTP/1.1\r\n\r\n", CharsetUtil.UTF_8)));
+
+        HttpRequest request = ch.readInbound();
+        assertEquals(HttpMethod.HEAD, request.method());
+        LastHttpContent content = ch.readInbound();
+        assertFalse(content.content().isReadable());
+        content.release();
+
+        HttpResponse response = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
+        HttpUtil.setTransferEncodingChunked(response, true);
+        assertTrue(ch.writeOutbound(response));
+        assertTrue(ch.writeOutbound(LastHttpContent.EMPTY_LAST_CONTENT));
+        assertTrue(ch.finish());
+
+        ByteBuf buf = ch.readOutbound();
+        assertEquals("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n", buf.toString(CharsetUtil.US_ASCII));
+        buf.release();
+
+        buf = ch.readOutbound();
+        assertFalse(buf.isReadable());
+        buf.release();
+
+        assertFalse(ch.finishAndReleaseAll());
+    }
+
     private static ByteBuf prepareDataChunk(int size) {
         StringBuilder sb = new StringBuilder();
         for (int i = 0; i < size; ++i) {