Repository: netty/netty
Issue: Make Http2UnknownFrame extend Http2StreamFrame rather than Http2Frame

This is a follow-up issue regarding: https://github.com/netty/netty/issues/7860

### Expected behavior

Writing an `Http2UnknownFrame` to a child channel will be written to the parent channel and underlying HTTP/2 connection via `Http2MultiplexCodec`.

When `Http2MultiplexCodec` reads an `Http2UnknownFrame`, it will be propagated to the correct child channel.

### Actual behavior

`Http2MultiplexCodec` rejects the write since `Http2UnknownFrame` is not an `instanceof Http2StreamFrame`.

https://github.com/netty/netty/blob/dfeb4b15b587bf5298555e969a01378853efab8f/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java#L1021-L1028

`Http2MultiplexCodec` does not propagate reads to child channels since `Http2UnknownFrame` is not an `instanceof Http2StreamFrame`.

https://github.com/netty/netty/blob/dfeb4b15b587bf5298555e969a01378853efab8f/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java#L219-L221

### Proposal

`Http2UnknownFrame` should extend `Http2StreamFrame`, rather than `Http2Frame`, as it already defines the two stream-related methods that the two interfaces differ in.

Should it possibly declare an `isEndStream` method as well, similar to `Http2DataFrame`?

Issue comments:
@Bennett-Lynch let it extend `Http2StreamFrame` sounds like the correct thing to do. Would you be interested in proving a PR ?

I think I would leave the `isEndStream` as it is and just use the `Http2Flags` for it.
@Bennett-Lynch I had a bit time this morning so I just did it myself and adding unit tests:

https://github.com/netty/netty/pull/7976

PTAL
Thanks, @normanmaurer -- change looks good to me.

Fine with using `Http2Flags` for detecting `isEndStream`, but may I ask your reasoning for it?  Maintaining a consistent interface across frame types seems more ideal, but I realize these UnknownFrames are a bit of an edge case.
@Bennett-Lynch because its redundant as it exists in the flags already (and you can have other flags as well).

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2UnknownFrame.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2UnknownFrame.java
index 51eeafaf0d9..41e578703a5 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2UnknownFrame.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2UnknownFrame.java
@@ -20,13 +20,12 @@
 import io.netty.util.internal.UnstableApi;
 
 @UnstableApi
-public interface Http2UnknownFrame extends Http2Frame, ByteBufHolder {
+public interface Http2UnknownFrame extends Http2StreamFrame, ByteBufHolder {
 
+    @Override
     Http2FrameStream stream();
 
-    /**
-     * Set the {@link Http2FrameStream} object for this frame.
-     */
+    @Override
     Http2UnknownFrame stream(Http2FrameStream stream);
 
     byte frameType();


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java
index 7a0e8c6a3a0..f07b218e095 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java
@@ -104,6 +104,43 @@ public void tearDown() throws Exception {
     // TODO(buchgr): GOAWAY Logic
     // TODO(buchgr): Test ChannelConfig.setMaxMessagesPerRead
 
+    @Test
+    public void writeUnknownFrame() {
+        childChannelInitializer.handler = new ChannelInboundHandlerAdapter() {
+            @Override
+            public void channelActive(ChannelHandlerContext ctx) {
+                ctx.writeAndFlush(new DefaultHttp2HeadersFrame(new DefaultHttp2Headers()));
+                ctx.writeAndFlush(new DefaultHttp2UnknownFrame((byte) 99, new Http2Flags()));
+                ctx.fireChannelActive();
+            }
+        };
+
+        Channel childChannel = newOutboundStream();
+        assertTrue(childChannel.isActive());
+
+        Http2FrameStream stream = readOutboundHeadersAndAssignId();
+        parentChannel.runPendingTasks();
+
+        Http2UnknownFrame frame = parentChannel.readOutbound();
+        assertEquals(stream, frame.stream());
+        assertEquals(99, frame.frameType());
+        assertEquals(new Http2Flags(), frame.flags());
+        frame.release();
+    }
+
+    @Test
+    public void readUnkownFrame() {
+        LastInboundHandler inboundHandler = streamActiveAndWriteHeaders(inboundStream);
+        codec.onHttp2Frame(new DefaultHttp2UnknownFrame((byte) 99, new Http2Flags()).stream(inboundStream));
+        codec.onChannelReadComplete();
+
+        // headers and unknown frame
+        verifyFramesMultiplexedToCorrectChannel(inboundStream, inboundHandler, 2);
+
+        Channel childChannel = newOutboundStream();
+        assertTrue(childChannel.isActive());
+    }
+
     @Test
     public void headerAndDataFramesShouldBeDelivered() {
         LastInboundHandler inboundHandler = new LastInboundHandler();