Repository: netty/netty
Issue: PongWebSocketFrames are inaccessible in handlers downstream of a WebSocketClientProtocolHandler

This is probably more of a question / request rather than a bug, as it looks like this was a deliberate decision ([source](https://github.com/netty/netty/blob/netty-4.1.25.Final/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandler.java#L32-L35)).

The use-case is a handler being able to assert that a pong frame was received from a Netty-based proxy server.

### Expected behavior

`PongWebSocketFrame`s are visible to a channel handler installed _downstream_ of a `WebSocketClientProtocolHandler`.

### Actual behavior

Sending a `PingWebSocketFrame` from a client to a server results in receipt of a `PongWebSocketFrame` by the client in the `WebSocketClientProtocolHandler`, but this frame is not propagated further along the channel pipeline.

### Steps to reproduce

I've pushed a commit [here](https://github.com/nicktrav/netty/commit/4cae21eb36fe873d24073e730cbda9bb1994741b) which demonstrates the issue.

### Minimal yet complete reproducer code (or URL to code)

Check out commit and run:
```bash
$ mvn test -pl codec-http -Dtest=io.netty.handler.codec.http.websocketx.PongFrameIgnoreTest
```

### Netty version

`4.1.25.Final`. Also run against `HEAD` of `4.1` branch @ 987c44388.

### JVM version (e.g. `java -version`)

```bash
$ java -version
java version "10.0.1" 2018-04-17
Java(TM) SE Runtime Environment 18.3 (build 10.0.1+10)
Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10.0.1+10, mixed mode)
```

### OS version (e.g. `uname -a`)

```bash
$ uname -a
Darwin nickt.local 17.3.0 Darwin Kernel Version 17.3.0: Thu Nov  9 18:09:22 PST 2017; root:xnu-4570.31.3~1/RELEASE_X86_64 x86_64
```


Issue comments:
@nicktrav actually I think the idea was that usually PONG frames should be dropped on the floor. That said I would argue that is not the best thing to do "magically" and it may be better to let the user decide to drop it or not. So maybe we should not do this ?
Yeah, that would be awesome for us. Principle of least surprise, etc, etc.

I might take a crack at this one, if that's cool? I was thinking I'd just remove those lines so the frames are propagated, and I'll add a couple tests for it in the server and client handler subclasses. Wdyt?
yeah... please :)
@normanmaurer wouldn't this cause regression? Maybe making `decode` method of `WebSocketServerProtocolHandler` public would be enough, so users can override this behavior?
@doom369 I think we should just do what @trustin suggested in the PR and make it configurable and use the old behaviour as default.
@nicktrav , @normanmaurer In our case we extend ``WebSocketServerProtocolHandler`` and override decode() for handling pong and close frames, but we can add some handler before ``WebSocketServerProtocolHandler`` and listen all web socket frames. Maybe in downstream handler anybody want listen close frame ??? 

Pull request review comments:
assert return value please
assert return value please
assert return value please
You can use use `Unpooled.copiedBuffer(...)` to simplify this a bit.
Call `channel.finish()` and assert return value
Call `channel.finish()` and assert return value
Call `channel.finish()` and assert return value
Use `CharsetUtil.UTF_8`
@normanmaurer I would suggest flipping the conditions, boolean is cheaper than instance of. 
@johnou I thought so as well when review but then just assumed it will be optimised any way as the JIT will know `dropPongFrames` will never change and so can just optimise the branch.
so to make it short I think it not matters.
:+1: makes sense 

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandler.java
index 2ace7e8aa09..7c85c57748d 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandler.java
@@ -151,6 +151,23 @@ public WebSocketClientProtocolHandler(URI webSocketURL, WebSocketVersion version
      *            {@code true} if close frames should not be forwarded and just close the channel
      */
     public WebSocketClientProtocolHandler(WebSocketClientHandshaker handshaker, boolean handleCloseFrames) {
+        this(handshaker, handleCloseFrames, true);
+    }
+
+    /**
+     * Base constructor
+     *
+     * @param handshaker
+     *            The {@link WebSocketClientHandshaker} which will be used to issue the handshake once the connection
+     *            was established to the remote peer.
+     * @param handleCloseFrames
+     *            {@code true} if close frames should not be forwarded and just close the channel
+     * @param dropPongFrames
+     *            {@code true} if pong frames should not be forwarded
+     */
+    public WebSocketClientProtocolHandler(WebSocketClientHandshaker handshaker, boolean handleCloseFrames,
+                                          boolean dropPongFrames) {
+        super(dropPongFrames);
         this.handshaker = handshaker;
         this.handleCloseFrames = handleCloseFrames;
     }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandler.java
index 7037ae34686..53532cafe98 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandler.java
@@ -22,6 +22,27 @@
 import java.util.List;
 
 abstract class WebSocketProtocolHandler extends MessageToMessageDecoder<WebSocketFrame> {
+
+    private final boolean dropPongFrames;
+
+    /**
+     * Creates a new {@link WebSocketProtocolHandler} that will <i>drop</i> {@link PongWebSocketFrame}s.
+     */
+    WebSocketProtocolHandler() {
+        this(true);
+    }
+
+    /**
+     * Creates a new {@link WebSocketProtocolHandler}, given a parameter that determines whether or not to drop {@link
+     * PongWebSocketFrame}s.
+     *
+     * @param dropPongFrames
+     *            {@code true} if {@link PongWebSocketFrame}s should be dropped
+     */
+    WebSocketProtocolHandler(boolean dropPongFrames) {
+        this.dropPongFrames = dropPongFrames;
+    }
+
     @Override
     protected void decode(ChannelHandlerContext ctx, WebSocketFrame frame, List<Object> out) throws Exception {
         if (frame instanceof PingWebSocketFrame) {
@@ -29,8 +50,7 @@ protected void decode(ChannelHandlerContext ctx, WebSocketFrame frame, List<Obje
             ctx.channel().writeAndFlush(new PongWebSocketFrame(frame.content()));
             return;
         }
-        if (frame instanceof PongWebSocketFrame) {
-            // Pong frames need to get ignored
+        if (frame instanceof PongWebSocketFrame && dropPongFrames) {
             return;
         }
 
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerProtocolHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerProtocolHandler.java
index 6b5ef13af53..3a19b145426 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerProtocolHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerProtocolHandler.java
@@ -132,6 +132,13 @@ public WebSocketServerProtocolHandler(String websocketPath, String subprotocols,
 
     public WebSocketServerProtocolHandler(String websocketPath, String subprotocols,
             boolean allowExtensions, int maxFrameSize, boolean allowMaskMismatch, boolean checkStartsWith) {
+        this(websocketPath, subprotocols, allowExtensions, maxFrameSize, allowMaskMismatch, checkStartsWith, true);
+    }
+
+    public WebSocketServerProtocolHandler(String websocketPath, String subprotocols,
+                                          boolean allowExtensions, int maxFrameSize, boolean allowMaskMismatch,
+                                          boolean checkStartsWith, boolean dropPongFrames) {
+        super(dropPongFrames);
         this.websocketPath = websocketPath;
         this.subprotocols = subprotocols;
         this.allowExtensions = allowExtensions;


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandlerTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandlerTest.java
new file mode 100644
index 00000000000..af7498222ce
--- /dev/null
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketProtocolHandlerTest.java
@@ -0,0 +1,89 @@
+/*
+ * 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.http.websocketx;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import io.netty.channel.embedded.EmbeddedChannel;
+import io.netty.util.CharsetUtil;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+/**
+ * Tests common, abstract class functionality in {@link WebSocketClientProtocolHandler}.
+ */
+public class WebSocketProtocolHandlerTest {
+
+    @Test
+    public void testPingFrame() {
+        ByteBuf pingData = Unpooled.copiedBuffer("Hello, world", CharsetUtil.UTF_8);
+        EmbeddedChannel channel = new EmbeddedChannel(new WebSocketProtocolHandler() { });
+
+        PingWebSocketFrame inputMessage = new PingWebSocketFrame(pingData);
+        assertFalse(channel.writeInbound(inputMessage)); // the message was not propagated inbound
+
+        // a Pong frame was written to the channel
+        PongWebSocketFrame response = channel.readOutbound();
+        assertEquals(pingData, response.content());
+
+        pingData.release();
+        assertFalse(channel.finish());
+    }
+
+    @Test
+    public void testPongFrameDropFrameFalse() {
+        EmbeddedChannel channel = new EmbeddedChannel(new WebSocketProtocolHandler(false) { });
+
+        PongWebSocketFrame pingResponse = new PongWebSocketFrame();
+        assertTrue(channel.writeInbound(pingResponse));
+
+        assertPropagatedInbound(pingResponse, channel);
+
+        pingResponse.release();
+        assertFalse(channel.finish());
+    }
+
+    @Test
+    public void testPongFrameDropFrameTrue() {
+        EmbeddedChannel channel = new EmbeddedChannel(new WebSocketProtocolHandler(true) { });
+
+        PongWebSocketFrame pingResponse = new PongWebSocketFrame();
+        assertFalse(channel.writeInbound(pingResponse)); // message was not propagated inbound
+    }
+
+    @Test
+    public void testTextFrame() {
+        EmbeddedChannel channel = new EmbeddedChannel(new WebSocketProtocolHandler() { });
+
+        TextWebSocketFrame textFrame = new TextWebSocketFrame();
+        assertTrue(channel.writeInbound(textFrame));
+
+        assertPropagatedInbound(textFrame, channel);
+
+        textFrame.release();
+        assertFalse(channel.finish());
+    }
+
+    /**
+     * Asserts that a message was propagated inbound through the channel.
+     */
+    private static <T extends WebSocketFrame> void assertPropagatedInbound(T message, EmbeddedChannel channel) {
+        T propagatedResponse = channel.readInbound();
+        assertEquals(message, propagatedResponse);
+    }
+}