[netty#4327] Ensure toString() will not throw IllegalReferenceCountException

normanmaurer · normanmaurer · commit 99b11c95b4d9 · 2015-10-10T20:12:43.000+02:00
Motivation:

As toString() is often used while logging we need to ensure this produces no exception.

Modifications:

Ensure we never throw an IllegalReferenceCountException.

Result:

Be able to log without produce exceptions.
diff --git a/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java b/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java
@@ -90,8 +90,16 @@ public boolean release(int decrement) {
         return data.release(decrement);
     }
 
+    /**
+     * Return {@link ByteBuf#toString()} without checking the reference count first. This is useful to implemement
+     * {@link #toString()}.
+     */
+    protected final String contentToString() {
+        return data.toString();
+    }
+
     @Override
     public String toString() {
-        return StringUtil.simpleClassName(this) + '(' + content().toString() + ')';
+        return StringUtil.simpleClassName(this) + '(' + contentToString() + ')';
     }
 }
diff --git a/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java b/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2015 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.buffer;
+
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+public class DefaultByteBufHolderTest {
+
+    @Test
+    public void testToString() {
+        ByteBufHolder holder = new DefaultByteBufHolder(Unpooled.buffer());
+        assertEquals(1, holder.refCnt());
+        assertNotNull(holder.toString());
+        assertTrue(holder.release());
+        assertNotNull(holder.toString());
+    }
+}
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketFrame.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketFrame.java
@@ -68,7 +68,7 @@ public int rsv() {
 
     @Override
     public String toString() {
-        return StringUtil.simpleClassName(this) + "(data: " + content() + ')';
+        return StringUtil.simpleClassName(this) + "(data: " + contentToString() + ')';
     }
 
     @Override
diff --git a/transport-sctp/src/main/java/io/netty/channel/sctp/SctpMessage.java b/transport-sctp/src/main/java/io/netty/channel/sctp/SctpMessage.java
@@ -17,7 +17,6 @@
 
 import com.sun.nio.sctp.MessageInfo;
 import io.netty.buffer.ByteBuf;
-import io.netty.buffer.ByteBufUtil;
 import io.netty.buffer.DefaultByteBufHolder;
 
 /**
@@ -191,15 +190,9 @@ public SctpMessage touch(Object hint) {
 
     @Override
     public String toString() {
-        if (refCnt() == 0) {
-            return "SctpFrame{" +
-                    "streamIdentifier=" + streamIdentifier + ", protocolIdentifier=" + protocolIdentifier +
-                    ", unordered=" + unordered +
-                    ", data=(FREED)}";
-        }
         return "SctpFrame{" +
-                "streamIdentifier=" + streamIdentifier + ", protocolIdentifier=" + protocolIdentifier +
-                ", unordered=" + unordered +
-                ", data=" + ByteBufUtil.hexDump(content()) + '}';
+               "streamIdentifier=" + streamIdentifier + ", protocolIdentifier=" + protocolIdentifier +
+               ", unordered=" + unordered +
+               ", data=" + contentToString() + '}';
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -90,8 +90,16 @@ public boolean release(int decrement) {`
`90`	`90`	`return data.release(decrement);`
`91`	`91`	`}`
`92`	`92`
	`93`	`+ /**`
	`94`	`+ * Return {@link ByteBuf#toString()} without checking the reference count first. This is useful to implemement`
	`95`	`+ * {@link #toString()}.`
	`96`	`+ */`
	`97`	`+ protected final String contentToString() {`
	`98`	`+ return data.toString();`
	`99`	`+ }`
	`100`	`+`
`93`	`101`	`@Override`
`94`	`102`	`public String toString() {`
`95`		`- return StringUtil.simpleClassName(this) + '(' + content().toString() + ')';`
	`103`	`+ return StringUtil.simpleClassName(this) + '(' + contentToString() + ')';`
`96`	`104`	`}`
`97`	`105`	`}`
Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,7 @@ public int rsv() {`
`68`	`68`
`69`	`69`	`@Override`
`70`	`70`	`public String toString() {`
`71`		`- return StringUtil.simpleClassName(this) + "(data: " + content() + ')';`
	`71`	`+ return StringUtil.simpleClassName(this) + "(data: " + contentToString() + ')';`
`72`	`72`	`}`
`73`	`73`
`74`	`74`	`@Override`