Repository: netty/netty
Issue: pooled ByteBuf copy will create unpooled ByteBuf
Labels: improvement

-Dio.netty.allocator.type : pooled

When i call CompositeByteBuf.copy , but it create a Unpooled ByteBuf.
and then my server frequently GC.
I think this method should be written like this :

`public ByteBuf copy(int index, int length) {`
`checkIndex(index, length);`
`ByteBuf dst = ByteBufAllocator.DEFAULT.buffer(length);`
`if (length != 0) {`
`copyTo(index, length, toComponentIndex(index), dst);`
`}`
`return dst;`
`    }`

This is just one of my Suggestions, if right, hope to fix it later. If not, please correct it.

Issue comments:
I think changing copy() to create pooled buffers would be api breaking and would cause memory leaks in user apps...(no need to call .release() right now)
Personally I avoid using copy() for the reason you mentioned and replace it with:
1. create new pooled buffer with the same size
2. newBuf.readBytes(buf)
I would also recommend avoiding CompositeByteBuf for most use cases, especially for high performance
@ninja- I disagree with your point about API breakage. You are supposed to call release() if you not do its a bug. That said I will need to have a look whats going on here.
@las1991 please check https://github.com/netty/netty/pull/7395 ... This should fix it
@normanmaurer thank you

Pull request review comments:
Maybe `allocBuffer(length)`?
@fenik17 good catch... let me do this and add one more assert in the tests 👍 

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
index da4876d837b..8a3681b5305 100644
--- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
@@ -1333,7 +1333,7 @@ public int setBytes(int index, FileChannel in, long position, int length) throws
     @Override
     public ByteBuf copy(int index, int length) {
         checkIndex(index, length);
-        ByteBuf dst = Unpooled.buffer(length);
+        ByteBuf dst = allocBuffer(length);
         if (length != 0) {
             copyTo(index, length, toComponentIndex(index), dst);
         }


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
index 0d3b5db3c9d..f51475bf9ae 100644
--- a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
@@ -1116,4 +1116,24 @@ public void testReleasesItsComponents() {
         assertEquals(0, buffer.refCnt());
     }
 
+    @Test
+    public void testAllocatorIsSameWhenCopy() {
+        testAllocatorIsSameWhenCopy(false);
+    }
+
+    @Test
+    public void testAllocatorIsSameWhenCopyUsingIndexAndLength() {
+        testAllocatorIsSameWhenCopy(true);
+    }
+
+    private void testAllocatorIsSameWhenCopy(boolean withIndexAndLength) {
+        ByteBuf buffer = newBuffer(8);
+        buffer.writeZero(4);
+        ByteBuf copy = withIndexAndLength ? buffer.copy(0, 4) : buffer.copy();
+        assertEquals(buffer, copy);
+        assertEquals(buffer.isDirect(), copy.isDirect());
+        assertSame(buffer.alloc(), copy.alloc());
+        buffer.release();
+        copy.release();
+    }
 }
diff --git a/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java b/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java
index dcbccacb669..e8dcec21020 100644
--- a/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java
+++ b/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java
@@ -15,6 +15,7 @@
  */
 package io.netty.buffer;
 
+import io.netty.util.CharsetUtil;
 import org.junit.Test;
 
 import static io.netty.buffer.Unpooled.*;
@@ -26,11 +27,13 @@
 public class ConsolidationTest {
     @Test
     public void shouldWrapInSequence() {
-        ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes()), wrappedBuffer("=".getBytes()));
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes()), wrappedBuffer("&".getBytes()));
+        ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII)));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII)));
 
         ByteBuf copy = currentBuffer.copy();
-        String s = new String(copy.array());
+        String s = copy.toString(CharsetUtil.US_ASCII);
         assertEquals("a=1&", s);
 
         currentBuffer.release();
@@ -39,23 +42,33 @@ public void shouldWrapInSequence() {
 
     @Test
     public void shouldConsolidationInSequence() {
-        ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes()), wrappedBuffer("=".getBytes()));
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes()), wrappedBuffer("&".getBytes()));
+        ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII)));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII)));
 
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("b".getBytes()), wrappedBuffer("=".getBytes()));
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("2".getBytes()), wrappedBuffer("&".getBytes()));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("b".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII)));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("2".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII)));
 
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("c".getBytes()), wrappedBuffer("=".getBytes()));
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("3".getBytes()), wrappedBuffer("&".getBytes()));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("c".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII)));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("3".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII)));
 
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("d".getBytes()), wrappedBuffer("=".getBytes()));
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("4".getBytes()), wrappedBuffer("&".getBytes()));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("d".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII)));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("4".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII)));
 
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("e".getBytes()), wrappedBuffer("=".getBytes()));
-        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("5".getBytes()), wrappedBuffer("&".getBytes()));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("e".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII)));
+        currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("5".getBytes(CharsetUtil.US_ASCII)),
+                wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII)));
 
         ByteBuf copy = currentBuffer.copy();
-        String s = new String(copy.array());
+        String s = copy.toString(CharsetUtil.US_ASCII);
         assertEquals("a=1&b=2&c=3&d=4&e=5&", s);
 
         currentBuffer.release();