From 130522be843b72801d9f58693f8e0e9c323abd28 Mon Sep 17 00:00:00 2001 From: nizarm Date: Fri, 23 Aug 2019 09:51:57 -0700 Subject: [PATCH] =?UTF-8?q?Correctly=20handle=20client=20side=20http2=20up?= =?UTF-8?q?grades=20when=20Http2FrameCodec=20=E2=80=A6(9495)=20(#9501)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation: In the release (4.1.37) we introduced Http2MultiplexHandler as a replacement of Http2MultiplexCodec. This did split the frame parsing from the multiplexing to allow a more flexible way to handle frames and to make the code cleaner. Unfortunally we did miss to special handle this in Http2ClientUpgradeCodec and so did not correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...). This did lead to the situation that we did not correctly receive the event on the Http2MultiplexHandler and so did not correctly created the Http2StreamChannel for the upgrade stream. Because of this we ended up with an NPE if a frame was dispatched to the upgrade stream later on. Modifications: - Correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...) Result: Fixes #9495. --- .../codec/http2/Http2ClientUpgradeCodec.java | 57 ++++++++++++++++--- .../http2/Http2ClientUpgradeCodecTest.java | 30 ++++++++-- 2 files changed, 73 insertions(+), 14 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodec.java index 3a7ffed51c..c38de971ae 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodec.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodec.java @@ -47,13 +47,14 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade private final String handlerName; private final Http2ConnectionHandler connectionHandler; private final ChannelHandler upgradeToHandler; + private final ChannelHandler http2MultiplexHandler; public Http2ClientUpgradeCodec(Http2FrameCodec frameCodec, ChannelHandler upgradeToHandler) { this(null, frameCodec, upgradeToHandler); } public Http2ClientUpgradeCodec(String handlerName, Http2FrameCodec frameCodec, ChannelHandler upgradeToHandler) { - this(handlerName, (Http2ConnectionHandler) frameCodec, upgradeToHandler); + this(handlerName, (Http2ConnectionHandler) frameCodec, upgradeToHandler, null); } /** @@ -66,6 +67,18 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade this((String) null, connectionHandler); } + /** + * Creates the codec using a default name for the connection handler when adding to the + * pipeline. + * + * @param connectionHandler the HTTP/2 connection handler + * @param http2MultiplexHandler the Http2 Multiplexer handler to work with Http2FrameCodec + */ + public Http2ClientUpgradeCodec(Http2ConnectionHandler connectionHandler, + Http2MultiplexHandler http2MultiplexHandler) { + this((String) null, connectionHandler, http2MultiplexHandler); + } + /** * Creates the codec providing an upgrade to the given handler for HTTP/2. * @@ -74,24 +87,38 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade * @param connectionHandler the HTTP/2 connection handler */ public Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler) { - this(handlerName, connectionHandler, connectionHandler); + this(handlerName, connectionHandler, connectionHandler, null); + } + + /** + * Creates the codec providing an upgrade to the given handler for HTTP/2. + * + * @param handlerName the name of the HTTP/2 connection handler to be used in the pipeline, + * or {@code null} to auto-generate the name + * @param connectionHandler the HTTP/2 connection handler + */ + public Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler, + Http2MultiplexHandler http2MultiplexHandler) { + this(handlerName, connectionHandler, connectionHandler, http2MultiplexHandler); } private Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler, ChannelHandler - upgradeToHandler) { + upgradeToHandler, Http2MultiplexHandler http2MultiplexHandler) { this.handlerName = handlerName; this.connectionHandler = requireNonNull(connectionHandler, "connectionHandler"); this.upgradeToHandler = requireNonNull(upgradeToHandler, "upgradeToHandler"); + this.http2MultiplexHandler = http2MultiplexHandler; } @Override + public CharSequence protocol() { return HTTP_UPGRADE_PROTOCOL_NAME; } @Override public Collection setUpgradeHeaders(ChannelHandlerContext ctx, - HttpRequest upgradeRequest) { + HttpRequest upgradeRequest) { CharSequence settingsValue = getSettingsHeaderValue(ctx); upgradeRequest.headers().set(HTTP_UPGRADE_SETTINGS_HEADER, settingsValue); return UPGRADE_HEADERS; @@ -99,12 +126,24 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade @Override public void upgradeTo(ChannelHandlerContext ctx, FullHttpResponse upgradeResponse) - throws Exception { - // Add the handler to the pipeline. - ctx.pipeline().addAfter(ctx.name(), handlerName, upgradeToHandler); + throws Exception { + try { + // Add the handler to the pipeline. + ctx.pipeline().addAfter(ctx.name(), handlerName, upgradeToHandler); - // Reserve local stream 1 for the response. - connectionHandler.onHttpClientUpgrade(); + // Add the Http2 Multiplex handler as this handler handle events produced by the connectionHandler. + // See https://github.com/netty/netty/issues/9495 + if (http2MultiplexHandler != null) { + final String name = ctx.pipeline().context(connectionHandler).name(); + ctx.pipeline().addAfter(name, null, http2MultiplexHandler); + } + + // Reserve local stream 1 for the response. + connectionHandler.onHttpClientUpgrade(); + } catch (Http2Exception e) { + ctx.fireExceptionCaught(e); + ctx.close(); + } } /** diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodecTest.java index 256dea30eb..33940a34d8 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodecTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodecTest.java @@ -32,26 +32,42 @@ public class Http2ClientUpgradeCodecTest { @Test public void testUpgradeToHttp2ConnectionHandler() throws Exception { - testUpgrade(new Http2ConnectionHandlerBuilder().server(false).frameListener(new Http2FrameAdapter()).build()); + testUpgrade(new Http2ConnectionHandlerBuilder().server(false).frameListener( + new Http2FrameAdapter()).build(), null); } @Test public void testUpgradeToHttp2FrameCodec() throws Exception { - testUpgrade(Http2FrameCodecBuilder.forClient().build()); + testUpgrade(Http2FrameCodecBuilder.forClient().build(), null); } @Test public void testUpgradeToHttp2MultiplexCodec() throws Exception { testUpgrade(Http2MultiplexCodecBuilder.forClient(new HttpInboundHandler()) - .withUpgradeStreamHandler(new ChannelHandler() { }).build()); + .withUpgradeStreamHandler(new ChannelHandler() { }).build(), null); } - private static void testUpgrade(Http2ConnectionHandler handler) throws Exception { + @Test + public void testUpgradeToHttp2FrameCodecWithMultiplexer() throws Exception { + testUpgrade(Http2FrameCodecBuilder.forClient().build(), + new Http2MultiplexHandler(new HttpInboundHandler(), new HttpInboundHandler())); + } + + private static void testUpgrade(Http2ConnectionHandler handler, Http2MultiplexHandler multiplexer) + throws Exception { FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.OPTIONS, "*"); EmbeddedChannel channel = new EmbeddedChannel(new ChannelHandler() { }); ChannelHandlerContext ctx = channel.pipeline().firstContext(); - Http2ClientUpgradeCodec codec = new Http2ClientUpgradeCodec("connectionHandler", handler); + + Http2ClientUpgradeCodec codec; + + if (multiplexer == null) { + codec = new Http2ClientUpgradeCodec("connectionHandler", handler); + } else { + codec = new Http2ClientUpgradeCodec("connectionHandler", handler, multiplexer); + } + codec.setUpgradeHeaders(ctx, request); // Flush the channel to ensure we write out all buffered data channel.flush(); @@ -59,6 +75,10 @@ public class Http2ClientUpgradeCodecTest { codec.upgradeTo(ctx, null); assertNotNull(channel.pipeline().get("connectionHandler")); + if (multiplexer != null) { + assertNotNull(channel.pipeline().get(Http2MultiplexHandler.class)); + } + assertTrue(channel.finishAndReleaseAll()); }