Repository: netty/netty
Issue: StompFrame.copy() does not copy headers 
Labels: defect

### Expected behavior
Call stompFrame.copy() do copies of content and headers.

### Actual behavior
After call stompFrame.copy() only content was copy.

### Minimal yet complete reproducer code (or URL to code)

 `StompFrame stompFrame = new DefaultStompFrame(StompCommand.CONNECT);`
 `stompFrame.headers().set(StompHeaders.HOST, "localhost");`
 `StompFrame copyFrame = stompFrame.copy();`
 `Assert.assertTrue(copyFrame.headers().isEmpty());`

### Netty version
4.1.20.Final

### JVM version (e.g. `java -version`)
java version "1.8.0_151"
Java(TM) SE Runtime Environment (build 1.8.0_151-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.151-b12, mixed mode)

### OS version (e.g. `uname -a`)
Darwin Kernel Version 16.7.0: Mon Nov 13 21:56:25 PST 2017; root:xnu-3789.72.11~1/RELEASE_X86_64 x86_64


Issue comments:
Fixed by https://github.com/netty/netty/pull/7562

Pull request review comments:
Please use the same strategy as https://github.com/netty/netty/commit/942b993f2b9781ff2126ff92a6be5b975dfc72ed#diff-17c17dd59c9912dd08e4dc95fa04bcab. In summary adding a `copy()` method on `DefaultStompHeaders` which leverages `DefaultHeaders#copy()`.
I looked to this strategy but `DefaultStompHeaders extends DefaultHeader implements StompHeaders` we can't add `StompHeaders#copy()` method it will be clashes. Maybe we can add method with name like a `StompHeaders#deepCopy()` or another way ?
We can't modify `StompHeaders` because it is an interface and that would be an API breaking change. However we can modify `DefaultStompHeaders` and do the following:

```java
    @Override
    public DefaultStompHeaders copy() {
        DefaultStompHeaders copy = new DefaultStompHeaders();
        copy.addImpl(this);
        return copy;
    }
```

We could also modify `DefaultHeaders` so we can consolidate the `copy.addImpl`, and any other work which maybe necessary in the future, but this maybe unnecessary.
So we can, but in `DefaultStompHeadersSubframe` used interface `StompHeaders`, how we can access to method that not present in interface without casting or other trick? Maybe I misunderstood you.
@Scottmitch I understand that you are busy, but I would like to quickly close this defect.
> how we can access to method that not present in interface without casting or other trick? 

Unfortunately before jdk8 (which provides the `default` key word) extending interfaces is not possible without breaking API changes. So we have a few options:

1. No API breaking changed but just add a `DefaultStompHeaders copy()` method (as suggested above)
- change the member variable in `DefaultStompHeadersSubframe` so it is declared as type `DefaultStompHeaders`.
- make the new constructor in `DefaultStompHeadersSubframe` which currently takes a `StompHeaders` instead take a `DefaultStompHeaders` and make it package private
2. "break" the StompHeaders API and add a `StompHeaders copy()` method (and also override this method in `DefaultStompHeaders` as suggested above).

@normanmaurer - my guess is you would prefer the "non-breaking" API change? However maybe its ok because its STOMP...
I think you also need to call `release()` on the frames ?
You mean that `release()` not affect a copied frame or that is work correct by spec ?
Basically add:

```java
sourceFrame.release();
copyFrame.release();
```

This is needed as `DefaultStompFrame` implementations `ByteBufHolder` to ensure there are no memory leaks.
@amizurov can you explain why the changes to this class are needed at all ? I think you can revert the changes
So, when we create `new DefaultStompFrame` in `DefaultStompFrame#replace(...)` method we pass `headers.copy()` to the constructor.

But not to this class
It's super class for `DefaultStompFrame`, else it's will be look like this: 
 ```java
DefaultStompFrame(StompCommand command, ByteBuf content, DefaultStompHeaders 
headers)  {
        super(command);

        if (headers != null) {
            this.headers.set(headers);
        }

        if (content == null) {
            throw new NullPointerException("content");
        }
        this.content = content;
    }
```

or many other variation, I think that is not very clear. Look at `DefaultFullHttpRequest` we try use same approach.
@normanmaurer - This was option (1) from https://github.com/netty/netty/pull/7562#discussion_r163063162 and is necessary so `DefaultStompFrame` can call `copy()` on the `DefaultStompHeaders` class (which isn't present in `StompHeaders`).
never mind missed it is the super-class. 

Patch that resolved the issue:
diff --git a/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompFrame.java b/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompFrame.java
index d113ed6f2b7..8df4c650439 100644
--- a/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompFrame.java
+++ b/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompFrame.java
@@ -31,10 +31,16 @@ public DefaultStompFrame(StompCommand command) {
     }
 
     public DefaultStompFrame(StompCommand command, ByteBuf content) {
-        super(command);
+        this(command, content, null);
+    }
+
+    DefaultStompFrame(StompCommand command, ByteBuf content, DefaultStompHeaders headers) {
+        super(command, headers);
+
         if (content == null) {
             throw new NullPointerException("content");
         }
+
         this.content = content;
     }
 
@@ -60,7 +66,7 @@ public StompFrame retainedDuplicate() {
 
     @Override
     public StompFrame replace(ByteBuf content) {
-        return new DefaultStompFrame(command, content);
+        return new DefaultStompFrame(command, content, headers.copy());
     }
 
     @Override
diff --git a/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeaders.java b/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeaders.java
index 80fccf0f668..e9ad5b8d5b3 100644
--- a/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeaders.java
+++ b/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeaders.java
@@ -16,14 +16,14 @@
 
 package io.netty.handler.codec.stomp;
 
-import io.netty.handler.codec.CharSequenceValueConverter;
-import io.netty.handler.codec.DefaultHeaders;
-import io.netty.handler.codec.HeadersUtils;
-
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map.Entry;
 
+import io.netty.handler.codec.CharSequenceValueConverter;
+import io.netty.handler.codec.DefaultHeaders;
+import io.netty.handler.codec.HeadersUtils;
+
 import static io.netty.util.AsciiString.CASE_INSENSITIVE_HASHER;
 import static io.netty.util.AsciiString.CASE_SENSITIVE_HASHER;
 
@@ -58,4 +58,11 @@ public boolean contains(CharSequence name, CharSequence value, boolean ignoreCas
         return contains(name, value,
                 ignoreCase ? CASE_INSENSITIVE_HASHER : CASE_SENSITIVE_HASHER);
     }
+
+    @Override
+    public DefaultStompHeaders copy() {
+        DefaultStompHeaders copyHeaders = new DefaultStompHeaders();
+        copyHeaders.addImpl(this);
+        return copyHeaders;
+    }
 }
diff --git a/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeadersSubframe.java b/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeadersSubframe.java
index ca7db33d13c..728a8894bea 100644
--- a/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeadersSubframe.java
+++ b/codec-stomp/src/main/java/io/netty/handler/codec/stomp/DefaultStompHeadersSubframe.java
@@ -24,13 +24,19 @@ public class DefaultStompHeadersSubframe implements StompHeadersSubframe {
 
     protected final StompCommand command;
     protected DecoderResult decoderResult = DecoderResult.SUCCESS;
-    protected final StompHeaders headers = new DefaultStompHeaders();
+    protected final DefaultStompHeaders headers;
 
     public DefaultStompHeadersSubframe(StompCommand command) {
+        this(command, null);
+    }
+
+    DefaultStompHeadersSubframe(StompCommand command, DefaultStompHeaders headers) {
         if (command == null) {
             throw new NullPointerException("command");
         }
+
         this.command = command;
+        this.headers = headers == null ? new DefaultStompHeaders() : headers;
     }
 
     @Override


Test patch:
diff --git a/codec-stomp/src/test/java/io/netty/handler/codec/stomp/DefaultStompFrameTest.java b/codec-stomp/src/test/java/io/netty/handler/codec/stomp/DefaultStompFrameTest.java
new file mode 100644
index 00000000000..c995ca7f1b8
--- /dev/null
+++ b/codec-stomp/src/test/java/io/netty/handler/codec/stomp/DefaultStompFrameTest.java
@@ -0,0 +1,55 @@
+/*
+ * Copyright 2018 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.stomp;
+
+import io.netty.util.AsciiString;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultStompFrameTest {
+
+    @Test
+    public void testStompFrameCopy() {
+        StompFrame sourceFrame = new DefaultStompFrame(StompCommand.CONNECT);
+
+        assertTrue(sourceFrame.headers().isEmpty());
+
+        sourceFrame.headers().set(StompHeaders.HOST, "localhost");
+
+        StompFrame copyFrame = sourceFrame.copy();
+
+        assertEquals(sourceFrame.headers(), copyFrame.headers());
+        assertEquals(sourceFrame.content(), copyFrame.content());
+
+        AsciiString copyHeaderName = new AsciiString("foo");
+        AsciiString copyHeaderValue = new AsciiString("bar");
+        copyFrame.headers().set(copyHeaderName, copyHeaderValue);
+
+        assertFalse(sourceFrame.headers().contains(copyHeaderName, copyHeaderValue));
+        assertTrue(copyFrame.headers().contains(copyHeaderName, copyHeaderValue));
+
+        assertEquals(1, sourceFrame.headers().size());
+        assertEquals(2, copyFrame.headers().size());
+        assertNotEquals(sourceFrame.headers(), copyFrame.headers());
+
+        assertTrue(sourceFrame.release());
+        assertTrue(copyFrame.release());
+    }
+}