Don't propagate Http2WindowUpdateFrame to the child channel / propagate Http2ResetFrame as user event when using Http2MultiplexHandler (#9290)

Motivation:

We should not propage Http2WindowUpdateFrames to the child channels at all as these are not really use-ful and should not be flow-controlled via `read()` anyway.  In the other hand Http2ResetFrame is very useful but should be propagated via an user event so the user is aware of it directly even if the user stops reading.

Modifications:

- Dont propagate Http2WindowUpdateFrames when using Http2MultiplexHandler
- Use user event for Http2ResetFrame when using Http2MultiplexHandler
- Adjust javadoc of Http2MultiplexHandler
- Add unit tests

Result:

Fixes https://github.com/netty/netty/pull/8889 and https://github.com/netty/netty/pull/7635

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexHandler.java

@@ -77,10 +77,10 @@ import static io.netty.handler.codec.http2.Http2Exception.connectionError;
  * <h3>Writability and Flow Control</h3>
  *
  * A child channel observes outbound/remote flow control via the channel's writability. A channel only becomes writable
- * when it maps to an active HTTP/2 stream and the stream's flow control window is greater than zero. A child channel
+ * when it maps to an active HTTP/2 stream . A child channel does not know about the connection-level flow control
- * does not know about the connection-level flow control window. {@link ChannelHandler}s are free to ignore the
+ * window. {@link ChannelHandler}s are free to ignore the channel's writability, in which case the excessive writes will
- * channel's writability, in which case the excessive writes will be buffered by the parent channel. It's important to
+ * be buffered by the parent channel. It's important to note that only {@link Http2DataFrame}s are subject to
- * note that only {@link Http2DataFrame}s are subject to HTTP/2 flow control.
+ * HTTP/2 flow control.
  */
 @UnstableApi
 public final class Http2MultiplexHandler extends Http2ChannelDuplexHandler {
@@ -154,14 +154,31 @@ public final class Http2MultiplexHandler extends Http2ChannelDuplexHandler {
     public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
         parentReadInProgress = true;
         if (msg instanceof Http2StreamFrame) {
+            if (msg instanceof Http2WindowUpdateFrame) {
+                // We dont want to propagate update frames to the user
+                return;
+            }
             Http2StreamFrame streamFrame = (Http2StreamFrame) msg;
             DefaultHttp2FrameStream s =
                     (DefaultHttp2FrameStream) streamFrame.stream();
-            ((AbstractHttp2StreamChannel) s.attachment).fireChildRead(streamFrame);
+
+            AbstractHttp2StreamChannel channel = (AbstractHttp2StreamChannel) s.attachment;
+            if (msg instanceof Http2ResetFrame) {
+                // Reset frames needs to be propagated via user events as these are not flow-controlled and so
+                // must not be controlled by suppressing channel.read() on the child channel.
+                channel.pipeline().fireUserEventTriggered(msg);
+
+                // RST frames will also trigger closing of the streams which then will call
+                // AbstractHttp2StreamChannel.streamClosed()
+            } else {
+                channel.fireChildRead(streamFrame);
+            }
             return;
         }
 
         if (msg instanceof Http2GoAwayFrame) {
+            // goaway frames will also trigger closing of the streams which then will call
+            // AbstractHttp2StreamChannel.streamClosed()
             onHttp2GoAwayFrame(ctx, (Http2GoAwayFrame) msg);
         }
 

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java

@@ -27,4 +27,14 @@ public class Http2MultiplexCodecTest extends Http2MultiplexTest<Http2FrameCodec>
     protected ChannelHandler newMultiplexer(TestChannelInitializer childChannelInitializer) {
         return null;
     }
+
+    @Override
+    protected boolean useUserEventForResetFrame() {
+        return false;
+    }
+
+    @Override
+    protected boolean ignoreWindowUpdateFrames() {
+        return false;
+    }
 }

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexHandlerTest.java

@@ -30,4 +30,14 @@ public class Http2MultiplexHandlerTest extends Http2MultiplexTest<Http2FrameCode
     protected ChannelHandler newMultiplexer(TestChannelInitializer childChannelInitializer) {
         return new Http2MultiplexHandler(childChannelInitializer, null);
     }
+
+    @Override
+    protected boolean useUserEventForResetFrame() {
+        return true;
+    }
+
+    @Override
+    protected boolean ignoreWindowUpdateFrames() {
+        return true;
+    }
 }

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexTest.java

@@ -857,7 +857,9 @@ public abstract class Http2MultiplexTest<C extends Http2FrameCodec> {
         assertEqualsAndRelease(dataFrame3, inboundHandler.<Http2DataFrame>readInbound());
         assertEqualsAndRelease(dataFrame4, inboundHandler.<Http2DataFrame>readInbound());
 
-        Http2ResetFrame resetFrame = inboundHandler.readInbound();
+        Http2ResetFrame resetFrame = useUserEventForResetFrame() ? inboundHandler.<Http2ResetFrame>readUserEvent() :
+                inboundHandler.<Http2ResetFrame>readInbound();
+
         assertEquals(childChannel.stream(), resetFrame.stream());
         assertEquals(Http2Error.NO_ERROR.code(), resetFrame.errorCode());
 
@@ -870,6 +872,33 @@ public abstract class Http2MultiplexTest<C extends Http2FrameCodec> {
         childChannel.closeFuture().syncUninterruptibly();
     }
 
+    protected abstract boolean useUserEventForResetFrame();
+
+    protected abstract boolean ignoreWindowUpdateFrames();
+
+    @Test
+    public void windowUpdateFrames() {
+        AtomicInteger numReads = new AtomicInteger(1);
+        LastInboundHandler inboundHandler = new LastInboundHandler();
+        Http2StreamChannel childChannel = newInboundStream(3, false, numReads, inboundHandler);
+
+        assertEquals(new DefaultHttp2HeadersFrame(request).stream(childChannel.stream()), inboundHandler.readInbound());
+
+        frameInboundWriter.writeInboundWindowUpdate(childChannel.stream().id(), 4);
+
+        Http2WindowUpdateFrame updateFrame = inboundHandler.readInbound();
+        if (ignoreWindowUpdateFrames()) {
+            assertNull(updateFrame);
+        } else {
+            assertEquals(new DefaultHttp2WindowUpdateFrame(4).stream(childChannel.stream()), updateFrame);
+        }
+
+        frameInboundWriter.writeInboundWindowUpdate(Http2CodecUtil.CONNECTION_STREAM_ID, 6);
+
+        assertNull(parentChannel.readInbound());
+        childChannel.close().syncUninterruptibly();
+    }
+
     @Test
     public void childQueueIsDrainedAndNewDataIsDispatchedInParentReadLoopAutoRead() {
         AtomicInteger numReads = new AtomicInteger(1);