Repository: netty/netty
Issue: DefaultFullHttp(Request|Response).hashCode() throws IllegalRefCountException
Labels: defect

Netty Version: master (latest snapshot a6816bd59ef2923d82af5b7a7f3b722507a56a3a)

I've notice that if a `DefaultFullHttpRequest` or `DefaultFullHttpResponse` have been released, then calls to `hashCode()` will throw an `IllegalRefCountException` [here](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java#L167) and [here](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java#L170). I was wondering if this is considered the correct behaviour?


Issue comments:
No this is a bug

> Am 08.11.2015 um 22:03 schrieb Brendt notifications@github.com:
> 
> Netty Version: master (latest snapshot a6816bd)
> 
> I've notice that if a DefaultFullHttpRequest or DefaultFullHttpResponse have been released, then calls to hashCode() will throw an IllegalRefCountException. I was wondering if this is considered the correct behaviour?
> 
> —
> Reply to this email directly or view it on GitHub.

Is it just a matter of returning 0 if the content has been freed? If so I could provide a PR for it.  I wonder what would happen to lookups for entries in a hash-based container (such as a HashMap<DefaultFullHttpRequest, SomeObject>)?

@blucas - Semantics would change (the lookup may fail the second time) if we use an arbitrary constant if the content has been released.  Perhaps we should cache some (or all) of the hash code that is considered "imitable". I can submit a PR to cleanup.

@Scottmitch @blucas not sure what is the best way to fix this but I think we should not throw for equals(....), toString() and hashCode().

@Scottmitch - To give you some background, I discovered this issue when attempting to clear out data stored in a `DefaultHttpDataFactory` for a given request which had already been freed. It threw the `IllegalRefCountException` when attempting to lookup a value which was keyed by `DefaultFullHttpRequest` [here](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java#L250).  Maybe it isn't a good idea to use `FullHttpRequest` or `FullHttpResponse` objects as keys in maps?

@normanmaurer - I agree, we shouldn't throw, regardless of what the fix is.

@blucas - In general the hashCode and equals were added for testing purposes.  However it is not unreasonable to use these items as keys in a map. For example maybe a cache may use a request as a key into a map. You have to be aware of with the current implementation is that the content is used in the hash code generation, and it may be an expensive operation.

We can discuss the `DefaultHttpDataFactory` details in another issue if that is OK?

See https://github.com/netty/netty/pull/4463

@Scottmitch - thanks for the quick PR.  With regards to `DefaultHttpDataFactory` I was just curious. No need to raise another issue unless you think it warrants one.

@blucas - If you are happy then I am happy :). Honestly I would need to look into `DefaultHttpDataFactory` in more detail to see if it is necessary to use the entire `Full` message or not.

Fixed by https://github.com/netty/netty/pull/4463. Thanks for reporting @blucas !


Pull request review comments:
nit: `e` -> `ignore`

nit: `e` -> `ignore`


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
index 87b2cf5e125..2ca9329e7c6 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
@@ -17,15 +17,19 @@
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
+import io.netty.util.IllegalReferenceCountException;
 
 /**
  * Default implementation of {@link FullHttpRequest}.
  */
 public class DefaultFullHttpRequest extends DefaultHttpRequest implements FullHttpRequest {
-    private static final int HASH_CODE_PRIME = 31;
     private final ByteBuf content;
     private final HttpHeaders trailingHeader;
     private final boolean validateHeaders;
+    /**
+     * Used to cache the value of the hash code and avoid {@link IllegalRefCountException}.
+     */
+    private int hash;
 
     public DefaultFullHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri) {
         this(httpVersion, method, uri, Unpooled.buffer(0));
@@ -163,11 +167,23 @@ public FullHttpRequest duplicate() {
 
     @Override
     public int hashCode() {
-        int result = 1;
-        result = HASH_CODE_PRIME * result + content().hashCode();
-        result = HASH_CODE_PRIME * result + trailingHeaders().hashCode();
-        result = HASH_CODE_PRIME * result + super.hashCode();
-        return result;
+        int hash = this.hash;
+        if (hash == 0) {
+            if (content().refCnt() != 0) {
+                try {
+                    hash = 31 + content().hashCode();
+                } catch (IllegalReferenceCountException ignored) {
+                    // Handle race condition between checking refCnt() == 0 and using the object.
+                    hash = 31;
+                }
+            } else {
+                hash = 31;
+            }
+            hash = 31 * hash + trailingHeaders().hashCode();
+            hash = 31 * hash + super.hashCode();
+            this.hash = hash;
+        }
+        return hash;
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
index 36ac176d640..5ad6058daf9 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
@@ -15,9 +15,11 @@
  */
 package io.netty.handler.codec.http;
 
-import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
+import io.netty.util.IllegalReferenceCountException;
+
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
 /**
  * Default implementation of a {@link FullHttpResponse}.
@@ -27,6 +29,10 @@ public class DefaultFullHttpResponse extends DefaultHttpResponse implements Full
     private final ByteBuf content;
     private final HttpHeaders trailingHeaders;
     private final boolean validateHeaders;
+    /**
+     * Used to cache the value of the hash code and avoid {@link IllegalRefCountException}.
+     */
+    private int hash;
 
     public DefaultFullHttpResponse(HttpVersion version, HttpResponseStatus status) {
         this(version, status, Unpooled.buffer(0));
@@ -164,6 +170,40 @@ public FullHttpResponse duplicate() {
         return duplicate;
     }
 
+    @Override
+    public int hashCode() {
+        int hash = this.hash;
+        if (hash == 0) {
+            if (content().refCnt() != 0) {
+                try {
+                    hash = 31 + content().hashCode();
+                } catch (IllegalReferenceCountException ignored) {
+                    // Handle race condition between checking refCnt() == 0 and using the object.
+                    hash = 31;
+                }
+            } else {
+                hash = 31;
+            }
+            hash = 31 * hash + trailingHeaders().hashCode();
+            hash = 31 * hash + super.hashCode();
+            this.hash = hash;
+        }
+        return hash;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+        if (!(o instanceof DefaultFullHttpResponse)) {
+            return false;
+        }
+
+        DefaultFullHttpResponse other = (DefaultFullHttpResponse) o;
+
+        return super.equals(other) &&
+               content().equals(other.content()) &&
+               trailingHeaders().equals(other.trailingHeaders());
+    }
+
     @Override
     public String toString() {
         return HttpMessageUtil.appendFullResponse(new StringBuilder(256), this).toString();


Test patch: