From 16b98d370f77b3ff389cbcdffee3b63779f0587a Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Thu, 4 Jul 2019 08:32:41 +0200
Subject: [PATCH] =?UTF-8?q?Correctly=20handle=20http2=20upgrades=20when=20?=
 =?UTF-8?q?Http2FrameCodec=20is=20used=20together=E2=80=A6=20(#9318)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 https://github.com/netty/netty/issues/9314.
---
 .../codec/http2/Http2ServerUpgradeCodec.java   | 18 +++++++++---------
 .../http2/Http2ServerUpgradeCodecTest.java     |  9 +++++++++
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
index 6b25e66efd..84911ecd4f 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
@@ -146,19 +146,19 @@ public class Http2ServerUpgradeCodec implements HttpServerUpgradeHandler.Upgrade
         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]);
-            }
         }
     }
 
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodecTest.java
index 02fa243629..50161bbdeb 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodecTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodecTest.java
@@ -76,6 +76,11 @@ public class Http2ServerUpgradeCodecTest {
         // 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 @@ public class Http2ServerUpgradeCodecTest {
         assertNotNull(settingsBuffer);
         settingsBuffer.release();
 
+        ByteBuf buf = channel.readOutbound();
+        assertNotNull(buf);
+        buf.release();
+
         assertNull(channel.readOutbound());
     }