Correctly handle http2 upgrades when Http2FrameCodec is used together… (netty#9318)

Motivation:

In the latest release 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 Http2ServerUpgradeCodec and so did not correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpServerUpgrade(...). 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.onHttpServerUpgrade(...)
- Add unit test

Result:

Fixes netty#9314.

Author: normanmaurer

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

@@ -146,19 +146,19 @@ public void upgradeTo(final ChannelHandlerContext ctx, FullHttpRequest upgradeRe
         try {
             // Add the HTTP/2 connection handler to the pipeline immediately following the current handler.
             ctx.pipeline().addAfter(ctx.name(), handlerName, connectionHandler);
-            connectionHandler.onHttpServerUpgrade(settings);
 
+            // Add also all extra handlers as these may handle events / messages produced by the connectionHandler.
+            // See https://github.com/netty/netty/issues/9314
+            if (handlers != null) {
+                final String name = ctx.pipeline().context(connectionHandler).name();
+                for (int i = handlers.length - 1; i >= 0; i--) {
+                    ctx.pipeline().addAfter(name, null, handlers[i]);
+                }
+            }
+            connectionHandler.onHttpServerUpgrade(settings);
         } catch (Http2Exception e) {
             ctx.fireExceptionCaught(e);
             ctx.close();
-            return;
-        }
-
-        if (handlers != null) {
-            final String name = ctx.pipeline().context(connectionHandler).name();
-            for (int i = handlers.length - 1; i >= 0; i--) {
-                ctx.pipeline().addAfter(name, null, handlers[i]);
-            }
         }
     }
 

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

@@ -76,6 +76,11 @@ private static void testUpgrade(Http2ConnectionHandler handler, ChannelHandler m
         // Flush the channel to ensure we write out all buffered data
         channel.flush();
 
+        channel.writeInbound(Http2CodecUtil.connectionPrefaceBuf());
+        Http2FrameInboundWriter writer = new Http2FrameInboundWriter(channel);
+        writer.writeInboundSettings(new Http2Settings());
+        writer.writeInboundRstStream(Http2CodecUtil.HTTP_UPGRADE_STREAM_ID, Http2Error.CANCEL.code());
+
         assertSame(handler, channel.pipeline().remove(handler.getClass()));
         assertNull(channel.pipeline().get(handler.getClass()));
         assertTrue(channel.finish());
@@ -85,6 +90,10 @@ private static void testUpgrade(Http2ConnectionHandler handler, ChannelHandler m
         assertNotNull(settingsBuffer);
         settingsBuffer.release();
 
+        ByteBuf buf = channel.readOutbound();
+        assertNotNull(buf);
+        buf.release();
+
         assertNull(channel.readOutbound());
     }