Repository: netty/netty
Issue: IllegalReferenceCountException using compression if HttpResponse implements HttpContent and not LastHttpContent

This was originally reported in Quarkus as https://github.com/quarkusio/quarkus/issues/14695. Vert.x uses https://github.com/eclipse-vertx/vert.x/blob/master/src/main/java/io/vertx/core/http/impl/AssembledHttpResponse.java which implements both HttpResponse and HttpContent, but not LastHttpContent.

This results in an IllegalReferenceCount exception in some situations when compression is enabled.

I am pretty sure the root cause is here: https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java#L196, as the buffer is not retained. 

Issue comments:

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 a0d951a16c5..9f94e025873 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
@@ -194,7 +194,7 @@ protected void encode(ChannelHandlerContext ctx, HttpObject msg, List<Object> ou
                     res.headers().remove(HttpHeaderNames.CONTENT_LENGTH);
                     res.headers().set(HttpHeaderNames.TRANSFER_ENCODING, HttpHeaderValues.CHUNKED);
 
-                    out.add(res);
+                    out.add(ReferenceCountUtil.retain(res));
                     state = State.AWAIT_CONTENT;
                     if (!(msg instanceof HttpContent)) {
                         // only break out the switch statement if we have not content to process


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
index c2bac013a10..901e424dd6a 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
@@ -15,6 +15,7 @@
  */
 package io.netty.handler.codec.http;
 
+import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufUtil;
 import io.netty.buffer.Unpooled;
 import io.netty.channel.embedded.EmbeddedChannel;
@@ -31,7 +32,11 @@
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 public class HttpContentCompressorTest {
 
@@ -151,6 +156,47 @@ public void testChunkedContent() throws Exception {
         assertThat(ch.readOutbound(), is(nullValue()));
     }
 
+    @Test
+    public void testChunkedContentWithAssembledResponse() throws Exception {
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpContentCompressor());
+        ch.writeInbound(newRequest());
+
+        HttpResponse res = new AssembledHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK,
+                Unpooled.copiedBuffer("Hell", CharsetUtil.US_ASCII));
+        res.headers().set(HttpHeaderNames.TRANSFER_ENCODING, HttpHeaderValues.CHUNKED);
+        ch.writeOutbound(res);
+
+        assertAssembledEncodedResponse(ch);
+
+        ch.writeOutbound(new DefaultHttpContent(Unpooled.copiedBuffer("o, w", CharsetUtil.US_ASCII)));
+        ch.writeOutbound(new DefaultLastHttpContent(Unpooled.copiedBuffer("orld", CharsetUtil.US_ASCII)));
+
+        HttpContent chunk;
+        chunk = ch.readOutbound();
+        assertThat(ByteBufUtil.hexDump(chunk.content()), is("1f8b0800000000000000f248cdc901000000ffff"));
+        chunk.release();
+
+        chunk = ch.readOutbound();
+        assertThat(ByteBufUtil.hexDump(chunk.content()), is("cad7512807000000ffff"));
+        chunk.release();
+
+        chunk = ch.readOutbound();
+        assertThat(ByteBufUtil.hexDump(chunk.content()), is("ca2fca4901000000ffff"));
+        chunk.release();
+
+        chunk = ch.readOutbound();
+        assertThat(ByteBufUtil.hexDump(chunk.content()), is("0300c2a99ae70c000000"));
+        assertThat(chunk, is(instanceOf(HttpContent.class)));
+        chunk.release();
+
+        chunk = ch.readOutbound();
+        assertThat(chunk.content().isReadable(), is(false));
+        assertThat(chunk, is(instanceOf(LastHttpContent.class)));
+        chunk.release();
+
+        assertThat(ch.readOutbound(), is(nullValue()));
+    }
+
     @Test
     public void testChunkedContentWithTrailingHeader() throws Exception {
         EmbeddedChannel ch = new EmbeddedChannel(new HttpContentCompressor());
@@ -550,4 +596,92 @@ private static void assertEncodedResponse(EmbeddedChannel ch) {
         assertThat(res.headers().get(HttpHeaderNames.CONTENT_LENGTH), is(nullValue()));
         assertThat(res.headers().get(HttpHeaderNames.CONTENT_ENCODING), is("gzip"));
     }
+    private static void assertAssembledEncodedResponse(EmbeddedChannel ch) {
+        Object o = ch.readOutbound();
+        assertThat(o, is(instanceOf(AssembledHttpResponse.class)));
+
+        AssembledHttpResponse res = (AssembledHttpResponse) o;
+        try {
+            assertThat(res, is(instanceOf(HttpContent.class)));
+            assertThat(res.headers().get(HttpHeaderNames.TRANSFER_ENCODING), is("chunked"));
+            assertThat(res.headers().get(HttpHeaderNames.CONTENT_LENGTH), is(nullValue()));
+            assertThat(res.headers().get(HttpHeaderNames.CONTENT_ENCODING), is("gzip"));
+        } finally {
+            res.release();
+        }
+    }
+
+    static class AssembledHttpResponse extends DefaultHttpResponse implements HttpContent {
+
+        private final ByteBuf content;
+
+        AssembledHttpResponse(HttpVersion version, HttpResponseStatus status, ByteBuf content) {
+            super(version, status);
+            this.content = content;
+        }
+
+        @Override
+        public HttpContent copy() {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public HttpContent duplicate() {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public HttpContent retainedDuplicate() {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public HttpContent replace(ByteBuf content) {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public AssembledHttpResponse retain() {
+            content.retain();
+            return this;
+        }
+
+        @Override
+        public AssembledHttpResponse retain(int increment) {
+            content.retain(increment);
+            return this;
+        }
+
+        @Override
+        public ByteBuf content() {
+            return content;
+        }
+
+        @Override
+        public int refCnt() {
+            return content.refCnt();
+        }
+
+        @Override
+        public boolean release() {
+            return content.release();
+        }
+
+        @Override
+        public boolean release(int decrement) {
+            return content.release(decrement);
+        }
+
+        @Override
+        public AssembledHttpResponse touch() {
+            content.touch();
+            return this;
+        }
+
+        @Override
+        public AssembledHttpResponse touch(Object hint) {
+            content.touch(hint);
+            return this;
+        }
+    }
 }