From 7dff856b1f97ce345140dd0944a744b6c03f6683 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 27 Jun 2019 21:52:52 +0200 Subject: [PATCH] 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/Http2MultiplexHandler.java | 27 +++++++++++++--- .../codec/http2/Http2MultiplexCodecTest.java | 10 ++++++ .../http2/Http2MultiplexHandlerTest.java | 10 ++++++ .../codec/http2/Http2MultiplexTest.java | 31 ++++++++++++++++++- 4 files changed, 72 insertions(+), 6 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexHandler.java index 300e37df3d..1c920b2ed3 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexHandler.java +++ b/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; *

Writability and Flow Control

* * 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 - * does not know about the connection-level flow control window. {@link ChannelHandler}s are free to ignore the - * channel's writability, in which case the excessive writes will be buffered by the parent channel. It's important to - * note that only {@link Http2DataFrame}s are subject to HTTP/2 flow control. + * when it maps to an active HTTP/2 stream . A child channel does not know about the connection-level flow control + * window. {@link ChannelHandler}s are free to ignore the channel's writability, in which case the excessive writes will + * be buffered by the parent channel. It's important to note that only {@link Http2DataFrame}s are subject to + * HTTP/2 flow control. */ @UnstableApi public final class Http2MultiplexHandler extends Http2ChannelDuplexHandler { @@ -159,14 +159,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); } 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 3f18918dc4..d9c20f9a83 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 @@ -27,4 +27,14 @@ public class Http2MultiplexCodecTest extends Http2MultiplexTest protected ChannelHandler newMultiplexer(TestChannelInitializer childChannelInitializer) { return null; } + + @Override + protected boolean useUserEventForResetFrame() { + return false; + } + + @Override + protected boolean ignoreWindowUpdateFrames() { + return false; + } } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexHandlerTest.java index 882d935f11..a37f74b75a 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexHandlerTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexHandlerTest.java @@ -30,4 +30,14 @@ public class Http2MultiplexHandlerTest extends Http2MultiplexTest { assertEqualsAndRelease(dataFrame3, inboundHandler.readInbound()); assertEqualsAndRelease(dataFrame4, inboundHandler.readInbound()); - Http2ResetFrame resetFrame = inboundHandler.readInbound(); + Http2ResetFrame resetFrame = useUserEventForResetFrame() ? inboundHandler.readUserEvent() : + inboundHandler.readInbound(); + assertEquals(childChannel.stream(), resetFrame.stream()); assertEquals(Http2Error.NO_ERROR.code(), resetFrame.errorCode()); @@ -888,6 +890,33 @@ public abstract class Http2MultiplexTest { 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);