Correctly handle client side http2 upgrades when Http2FrameCodec …(94…

…95) (#9501)

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/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 Http2ClientUpgradeCodec(Http2ConnectionHandler connectionHandler) {
         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,37 +87,63 @@ public Http2ClientUpgradeCodec(Http2ConnectionHandler connectionHandler) {
      * @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<CharSequence> setUpgradeHeaders(ChannelHandlerContext ctx,
-            HttpRequest upgradeRequest) {
+        HttpRequest upgradeRequest) {
         CharSequence settingsValue = getSettingsHeaderValue(ctx);
         upgradeRequest.headers().set(HTTP_UPGRADE_SETTINGS_HEADER, settingsValue);
         return UPGRADE_HEADERS;
     }
 
     @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);
+
+            // 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();
+            // Reserve local stream 1 for the response.
+            connectionHandler.onHttpClientUpgrade();
+        } catch (Http2Exception e) {
+            ctx.fireExceptionCaught(e);
+            ctx.close();
+        }
     }
 
     /**

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

@@ -32,33 +32,53 @@ 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();
 
         codec.upgradeTo(ctx, null);
         assertNotNull(channel.pipeline().get("connectionHandler"));
 
+        if (multiplexer != null) {
+            assertNotNull(channel.pipeline().get(Http2MultiplexHandler.class));
+        }
+
         assertTrue(channel.finishAndReleaseAll());
     }