Repository: netty/netty
Issue: AggregatedFullHttpMessage toString() behaviour
Labels: defect

This is related to #3019, the previous problem persists:

Prior to v4.0.24-FINAL you could call .toString on the output of HttpObjectAggregator regardless of whether the resulting HttpMessage has been released or not, from v.4.0.24-FINAL onwards you will encounter an error similar to:

```
io.netty.util.IllegalReferenceCountException: refCnt: 0
    at io.netty.buffer.DefaultByteBufHolder.content(DefaultByteBufHolder.java:39) ~[netty-all-4.0.27.Final.jar:4.0.27.Final]
    at io.netty.handler.codec.http.HttpMessageUtil.appendFullCommon(HttpMessageUtil.java:79) ~[netty-all-4.0.27.Final.jar:4.0.27.Final]
    at io.netty.handler.codec.http.HttpMessageUtil.appendFullRequest(HttpMessageUtil.java:55) ~[netty-all-4.0.27.Final.jar:4.0.27.Final]
```

This is due to the fact that AggregatedFullHttpMessage overrides .copy, .duplicate, and .retain from the ByteBufHolder interface but delegates .content to the DefaultByteBufHolder superclass which  checks the ref count every time you access content:

``` java
@Override
public ByteBuf content() {
    if (data.refCnt() <= 0) {
        throw new IllegalReferenceCountException(data.refCnt());
    }
    return data;
}
```

whereas something like DefaultHttpContent does not check that count:

``` java
@Override
public ByteBuf content() {
    return content;
}
```

It appears the AggregateFullHttpMessage only takes the benefit of .copy and .release from that type hierarchy, so I offer a PR where that is removed and the impl is similar internally to DefaultHttpContent, apologies if I've missed something obvious.

**context**

I have a Clojure networking framework which combines netty, clojure/core.async, and stuartsierra/component in an attempt at an async, event-driven architecture.

Basically you can combine components using core.async channels and macros, netty is plugged in as the network layer which translates input/output events to/from requests/responses. Any netty codec would work but in reality I use a fairly standard HTTP channel pipeline most often, and HttpObjectAggregator features in that.

I can manage the lifecycle of pooled bytebuf cleanly, or for some applications when each request is decoded and converted to an event I copy the bytebuf into memory as a single java.util.ByteBuffer and release the original at that point. In that case I still place the request alongside the ByteBuffer on the event, making it available for further processing - i.e. decoding the query-string or http-headers.

On occasion events are logged to disk, which calls toString, which errors if the msg has been released, where previously it did not.

There are multiple solutions to my problem, I could always manage pooled buffers and not memory copy, I could always configure my server to use Unpooled buffers, but all the other HttpMessage impl seem to be fine with .toString when released and the original behaviour has changed so I thought I'd chip in again.

Ta,
 Derek


Issue comments:
Fixed by #3638


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
index 9a504669502..415e2a849d0 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
@@ -16,6 +16,7 @@
 package io.netty.handler.codec.http;
 
 import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufHolder;
 import io.netty.buffer.CompositeByteBuf;
 import io.netty.buffer.DefaultByteBufHolder;
 import io.netty.buffer.Unpooled;
@@ -277,13 +278,14 @@ private static FullHttpMessage toFullMessage(HttpMessage msg) {
         return fullMsg;
     }
 
-    private abstract static class AggregatedFullHttpMessage extends DefaultByteBufHolder implements FullHttpMessage {
+    private abstract static class AggregatedFullHttpMessage implements ByteBufHolder, FullHttpMessage {
         protected final HttpMessage message;
+        private final ByteBuf content;
         private HttpHeaders trailingHeaders;
 
         AggregatedFullHttpMessage(HttpMessage message, ByteBuf content, HttpHeaders trailingHeaders) {
-            super(content);
             this.message = message;
+            this.content = content;
             this.trailingHeaders = trailingHeaders;
         }
 
@@ -328,17 +330,37 @@ public void setDecoderResult(DecoderResult result) {
         }
 
         @Override
-        public FullHttpMessage retain(int increment) {
-            super.retain(increment);
-            return this;
+        public ByteBuf content() {
+            return content;
+        }
+
+        @Override
+        public int refCnt() {
+            return content.refCnt();
         }
 
         @Override
         public FullHttpMessage retain() {
-            super.retain();
+            content.retain();
+            return this;
+        }
+
+        @Override
+        public FullHttpMessage retain(int increment) {
+            content.retain(increment);
             return this;
         }
 
+        @Override
+        public boolean release() {
+            return content.release();
+        }
+
+        @Override
+        public boolean release(int decrement) {
+            return content.release(decrement);
+        }
+
         @Override
         public abstract FullHttpMessage copy();
 


Test patch: