Repository: netty/netty
Issue: codec-memcache: copy, duplicate, replace not copying full request/response metadata

### Expected behavior

Calling `copy()`, `duplicate()` or `replace()` on `FullBinaryMemcacheResponse` or `FullBinaryMemcacheRequest` instances should copy status, opCode, etc. that are defined in `AbstractBinaryMemcacheMessage`.

### Actual behavior

Calling these methods do not copy the data and status, opCode etc. get lost upon calling these methods.

### Steps to reproduce

1. Create a `DefaultFullBinaryMemcacheRequest/Response`.
2. Set status, or opCode, ...
3. Call `copy()`
4. Notice copied instance is missing status, opCode, etc.

### Minimal yet complete reproducer code (or URL to code)

Unit test:
```
    @Test
    public void test() {
        DefaultFullBinaryMemcacheResponse res = new DefaultFullBinaryMemcacheResponse(
                Unpooled.copiedBuffer("key", CharsetUtil.UTF_8),
                Unpooled.EMPTY_BUFFER,
                Unpooled.EMPTY_BUFFER);
        res.setStatus((short) 1);
        res.setOpcode((byte) 0x01);
        FullBinaryMemcacheResponse copy = res.copy();
        assertEquals(1, copy.status());
        assertEquals(0x01, copy.opcode());
    }
```

### Netty version

Latest master.

### JVM version (e.g. `java -version`)

`java version "1.8.0_181"`

### OS version (e.g. `uname -a`)

`Darwin computer-name 17.7.0 Darwin Kernel Version 17.7.0: Wed Feb 27 00:43:23 PST 2019; root:xnu-4570.71.35~1/RELEASE_X86_64 x86_64`

Issue comments:
PR submitted: https://github.com/netty/netty/pull/9160

Pull request review comments:
2019
just use `assertEquals(expected.key(), actual.key()` .... same for the other lines 
call `expected.release()` and `actual.release()` in a finally block. 
2019
see above
see above
package-private ? 

please add javadocs
Addressed
Addressed
Getting a ref count exception when running mvn test when I do that. The buffers are unpooled.
Addressed
Addressed
Addressed
Addressed
hmm can I see the exception ?
ah I now see why... you sometimes pass the same buffer / request .

Just change it to when you pass in the same instance to use `retainedDuplicate()`. This will ensure you will also increment the reference count 
Addressed.
I've added some code to track what refs should be released and cleaned up in a junit `@After` method.
missing `@Override` ?
missing `@Override` ?
honestly I think it would be a lot cleaner if you just release stuff in the tests itself. 
See above... I would prefer if you just do it directly in the tests 
no the signature of the methods are different:
```
super: void copyMeta(AbstractBinaryMemcacheMessage dst)
this : void copyMeta(DefaultBinaryMemcacheRequest dst)
```
no the signature of the methods are different:
```
super: void copyMeta(AbstractBinaryMemcacheMessage dst)
this : void copyMeta(DefaultBinaryMemcacheResponse dst)
```
sure...
done.

Patch that resolved the issue:
diff --git a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheMessage.java b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheMessage.java
index c40ddf48d10..c9417e9730d 100644
--- a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheMessage.java
+++ b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheMessage.java
@@ -78,7 +78,7 @@ public BinaryMemcacheMessage setKey(ByteBuf key) {
         this.key = key;
         short oldKeyLength = keyLength;
         keyLength = key == null ? 0 : (short) key.readableBytes();
-        totalBodyLength  = totalBodyLength + keyLength - oldKeyLength;
+        totalBodyLength = totalBodyLength + keyLength - oldKeyLength;
         return this;
     }
 
@@ -232,4 +232,20 @@ public BinaryMemcacheMessage touch(Object hint) {
         }
         return this;
     }
+
+    /**
+     * Copies special metadata hold by this instance to the provided instance
+     *
+     * @param dst The instance where to copy the metadata of this instance to
+     */
+    void copyMeta(AbstractBinaryMemcacheMessage dst) {
+        dst.magic = magic;
+        dst.opcode = opcode;
+        dst.keyLength = keyLength;
+        dst.extrasLength = extrasLength;
+        dst.dataType = dataType;
+        dst.totalBodyLength = totalBodyLength;
+        dst.opaque = opaque;
+        dst.cas = cas;
+    }
 }
diff --git a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheRequest.java b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheRequest.java
index 68feb03a30e..bac5845510a 100644
--- a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheRequest.java
+++ b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheRequest.java
@@ -92,4 +92,14 @@ public BinaryMemcacheRequest touch(Object hint) {
         super.touch(hint);
         return this;
     }
+
+    /**
+     * Copies special metadata hold by this instance to the provided instance
+     *
+     * @param dst The instance where to copy the metadata of this instance to
+     */
+    void copyMeta(DefaultBinaryMemcacheRequest dst) {
+        super.copyMeta(dst);
+        dst.reserved = reserved;
+    }
 }
diff --git a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheResponse.java b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheResponse.java
index b632dcea849..639913edd9a 100644
--- a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheResponse.java
+++ b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheResponse.java
@@ -41,7 +41,7 @@ public DefaultBinaryMemcacheResponse() {
     /**
      * Create a new {@link DefaultBinaryMemcacheResponse} with the header and key.
      *
-     * @param key    the key to use
+     * @param key    the key to use.
      */
     public DefaultBinaryMemcacheResponse(ByteBuf key) {
         this(key, null);
@@ -92,4 +92,14 @@ public BinaryMemcacheResponse touch(Object hint) {
         super.touch(hint);
         return this;
     }
+
+    /**
+     * Copies special metadata hold by this instance to the provided instance
+     *
+     * @param dst The instance where to copy the metadata of this instance to
+     */
+    void copyMeta(DefaultBinaryMemcacheResponse dst) {
+        super.copyMeta(dst);
+        dst.status = status;
+    }
 }
diff --git a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequest.java b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequest.java
index dbc5bacbab4..8f6302dff29 100644
--- a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequest.java
+++ b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequest.java
@@ -102,7 +102,7 @@ public FullBinaryMemcacheRequest copy() {
         if (extras != null) {
             extras = extras.copy();
         }
-        return new DefaultFullBinaryMemcacheRequest(key, extras, content().copy());
+        return newInstance(key, extras, content().copy());
     }
 
     @Override
@@ -115,7 +115,7 @@ public FullBinaryMemcacheRequest duplicate() {
         if (extras != null) {
             extras = extras.duplicate();
         }
-        return new DefaultFullBinaryMemcacheRequest(key, extras, content().duplicate());
+        return newInstance(key, extras, content().duplicate());
     }
 
     @Override
@@ -133,6 +133,12 @@ public FullBinaryMemcacheRequest replace(ByteBuf content) {
         if (extras != null) {
             extras = extras.retainedDuplicate();
         }
-        return new DefaultFullBinaryMemcacheRequest(key, extras, content);
+        return newInstance(key, extras, content);
+    }
+
+    private DefaultFullBinaryMemcacheRequest newInstance(ByteBuf key, ByteBuf extras, ByteBuf content) {
+        DefaultFullBinaryMemcacheRequest newInstance = new DefaultFullBinaryMemcacheRequest(key, extras, content);
+        copyMeta(newInstance);
+        return newInstance;
     }
 }
diff --git a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponse.java b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponse.java
index 734cba8ffe9..a8853a58cca 100644
--- a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponse.java
+++ b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponse.java
@@ -102,7 +102,7 @@ public FullBinaryMemcacheResponse copy() {
         if (extras != null) {
             extras = extras.copy();
         }
-        return new DefaultFullBinaryMemcacheResponse(key, extras, content().copy());
+        return newInstance(key, extras, content().copy());
     }
 
     @Override
@@ -115,7 +115,7 @@ public FullBinaryMemcacheResponse duplicate() {
         if (extras != null) {
             extras = extras.duplicate();
         }
-        return new DefaultFullBinaryMemcacheResponse(key, extras, content().duplicate());
+        return newInstance(key, extras, content().duplicate());
     }
 
     @Override
@@ -133,6 +133,12 @@ public FullBinaryMemcacheResponse replace(ByteBuf content) {
         if (extras != null) {
             extras = extras.retainedDuplicate();
         }
-        return new DefaultFullBinaryMemcacheResponse(key, extras, content);
+        return newInstance(key, extras, content);
+    }
+
+    private FullBinaryMemcacheResponse newInstance(ByteBuf key, ByteBuf extras, ByteBuf content) {
+        DefaultFullBinaryMemcacheResponse newInstance = new DefaultFullBinaryMemcacheResponse(key, extras, content);
+        copyMeta(newInstance);
+        return newInstance;
     }
 }


Test patch:
diff --git a/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java b/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java
new file mode 100644
index 00000000000..6b7e98502d9
--- /dev/null
+++ b/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java
@@ -0,0 +1,99 @@
+/*
+ * Copyright 2019 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.handler.codec.memcache.binary;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import io.netty.util.CharsetUtil;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotSame;
+
+public class DefaultFullBinaryMemcacheRequestTest {
+
+    private DefaultFullBinaryMemcacheRequest request;
+
+    @Before
+    public void setUp() {
+        request = new DefaultFullBinaryMemcacheRequest(
+                Unpooled.copiedBuffer("key", CharsetUtil.UTF_8),
+                Unpooled.wrappedBuffer(new byte[]{1, 3, 4, 9}),
+                Unpooled.copiedBuffer("some value", CharsetUtil.UTF_8));
+        request.setReserved((short) 534);
+        request.setMagic((byte) 0x03);
+        request.setOpcode((byte) 0x02);
+        request.setKeyLength((short) 32);
+        request.setExtrasLength((byte) 34);
+        request.setDataType((byte) 43);
+        request.setTotalBodyLength(345);
+        request.setOpaque(3);
+        request.setCas(345345L);
+    }
+
+    @Test
+    public void fullCopy() {
+        FullBinaryMemcacheRequest newInstance = request.copy();
+        try {
+            assertCopy(request, request.content(), newInstance);
+        } finally {
+            request.release();
+            newInstance.release();
+        }
+    }
+
+    @Test
+    public void fullDuplicate() {
+        FullBinaryMemcacheRequest newInstance = request.duplicate();
+        try {
+            assertCopy(request, request.content(), newInstance);
+        } finally {
+            request.release();
+        }
+    }
+
+    @Test
+    public void fullReplace() {
+        ByteBuf newContent = Unpooled.copiedBuffer("new value", CharsetUtil.UTF_8);
+        FullBinaryMemcacheRequest newInstance = request.replace(newContent);
+        try {
+            assertCopy(request, newContent, newInstance);
+        } finally {
+            request.release();
+            newInstance.release();
+        }
+    }
+
+    private void assertCopy(FullBinaryMemcacheRequest expected, ByteBuf expectedContent,
+                            FullBinaryMemcacheRequest actual) {
+        assertNotSame(expected, actual);
+
+        assertEquals(expected.key(), actual.key());
+        assertEquals(expected.extras(), actual.extras());
+        assertEquals(expectedContent, actual.content());
+
+        assertEquals(expected.reserved(), actual.reserved());
+        assertEquals(expected.magic(), actual.magic());
+        assertEquals(expected.opcode(), actual.opcode());
+        assertEquals(expected.keyLength(), actual.keyLength());
+        assertEquals(expected.extrasLength(), actual.extrasLength());
+        assertEquals(expected.dataType(), actual.dataType());
+        assertEquals(expected.totalBodyLength(), actual.totalBodyLength());
+        assertEquals(expected.opaque(), actual.opaque());
+        assertEquals(expected.cas(), actual.cas());
+    }
+}
diff --git a/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponseTest.java b/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponseTest.java
new file mode 100644
index 00000000000..f94989ba281
--- /dev/null
+++ b/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponseTest.java
@@ -0,0 +1,98 @@
+/*
+ * Copyright 2019 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.handler.codec.memcache.binary;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import io.netty.util.CharsetUtil;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotSame;
+
+public class DefaultFullBinaryMemcacheResponseTest {
+
+    private DefaultFullBinaryMemcacheResponse response;
+
+    @Before
+    public void setUp() {
+        response = new DefaultFullBinaryMemcacheResponse(
+                Unpooled.copiedBuffer("key", CharsetUtil.UTF_8),
+                Unpooled.wrappedBuffer(new byte[]{1, 3, 4, 9}),
+                Unpooled.copiedBuffer("some value", CharsetUtil.UTF_8));
+        response.setStatus((short) 1);
+        response.setMagic((byte) 0x03);
+        response.setOpcode((byte) 0x02);
+        response.setKeyLength((short) 32);
+        response.setExtrasLength((byte) 34);
+        response.setDataType((byte) 43);
+        response.setTotalBodyLength(345);
+        response.setOpaque(3);
+        response.setCas(345345L);
+    }
+
+    @Test
+    public void fullCopy() {
+        FullBinaryMemcacheResponse newInstance = response.copy();
+        try {
+            assertResponseEquals(response, response.content(), newInstance);
+        } finally {
+            response.release();
+            newInstance.release();
+        }
+    }
+
+    @Test
+    public void fullDuplicate() {
+        try {
+            assertResponseEquals(response, response.content(), response.duplicate());
+        } finally {
+            response.release();
+        }
+    }
+
+    @Test
+    public void fullReplace() {
+        ByteBuf newContent = Unpooled.copiedBuffer("new value", CharsetUtil.UTF_8);
+        FullBinaryMemcacheResponse newInstance = response.replace(newContent);
+        try {
+            assertResponseEquals(response, newContent, newInstance);
+        } finally {
+            response.release();
+            newInstance.release();
+        }
+    }
+
+    private void assertResponseEquals(FullBinaryMemcacheResponse expected, ByteBuf expectedContent,
+                                      FullBinaryMemcacheResponse actual) {
+        assertNotSame(expected, actual);
+
+        assertEquals(expected.key(), actual.key());
+        assertEquals(expected.extras(), actual.extras());
+        assertEquals(expectedContent, actual.content());
+
+        assertEquals(expected.status(), actual.status());
+        assertEquals(expected.magic(), actual.magic());
+        assertEquals(expected.opcode(), actual.opcode());
+        assertEquals(expected.keyLength(), actual.keyLength());
+        assertEquals(expected.extrasLength(), actual.extrasLength());
+        assertEquals(expected.dataType(), actual.dataType());
+        assertEquals(expected.totalBodyLength(), actual.totalBodyLength());
+        assertEquals(expected.opaque(), actual.opaque());
+        assertEquals(expected.cas(), actual.cas());
+    }
+}