Repository: netty/netty
Issue: NullPointerException during Client Side ClearText Upgrade with new Http2MultiplexHandler.java

When cleartext upgrade is implemented in client side using **HttpClientUpgradeHandler** there is no way to pass the newly created **Http2MultiplexHandler**. 

It seems like we need a way to correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...). Since this special case is not handled in HttpClientUpgradeHandler (but seems like this case is correctly taken care in Http2ServerUpgradeCodec). 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.

Following is the exception I am receiving when I setup the Http2MultiplexHandler after the upgrade is taken place

va.lang.NullPointerException
        at io.netty.handler.codec.http2.AbstractHttp2StreamChannel$1.visit(AbstractHttp2StreamChannel.java:64) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
        at io.netty.handler.codec.http2.Http2FrameCodec$2.visit(Http2FrameCodec.java:200) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
        at io.netty.handler.codec.http2.DefaultHttp2Connection$ActiveStreams.forEachActiveStream(DefaultHttp2Connection.java:971) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
        at io.netty.handler.codec.http2.DefaultHttp2Connection.forEachActiveStream(DefaultHttp2Connection.java:208) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
        at io.netty.handler.codec.http2.Http2FrameCodec.forEachActiveStream(Http2FrameCodec.java:196) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
        at io.netty.handler.codec.http2.Http2ChannelDuplexHandler.forEachActiveStream(Http2ChannelDuplexHandler.java:83) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
        at io.netty.handler.codec.http2.Http2MultiplexHandler.channelWritabilityChanged(Http2MultiplexHandler.java:199) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelWritabilityChanged(AbstractChannelHandlerContext.java:436) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]

My Code looks something like this
```java
    Http2ClientUpgradeCodec upgradeCodec = new Http2ClientUpgradeCodec(
        "Http2Codec",
        Http2FrameCodecBuilder
        .forClient()
        .initialSettings(createHttp2Settings())
        .build()
        );

    channel.pipeline().addLast(sourceCodec);
    channel.pipeline().addLast(new HttpClientUpgradeHandler(sourceCodec, upgradeCodec, _maxContentLength));
    channel.pipeline().addLast(new Http2ProtocolUpgradeHandler(upgradePromise));
```
I add the Http2MultiplexHandler inside my Http2ProtocolUpgradeHandler during success of cleartext upgrade (as I dont have any other way to add it along with upgradecodec).

Netty Version I used : 4.1.39

When I follow the same approach to fix the serverside upgrade issue as mentioned in 
https://github.com/netty/netty/issues/9314 - it works perfectly form me.

@normanmaurer , @trustin  - let me know if this is a known bug and we are good with my proposed fix. I can give a pull request !

Issue comments:
@nizarm sounds like a bug yes... I would be happy to review a PR.
Thanks for confirming @normanmaurer - PR is ready for your review !

Pull request review comments:
I wonder if we should make this more consistent with what we have on the server side:

https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java#L107

Basically:

public Http2ClientUpgradeCodec(Http2FrameCodec http2Codec, ChannelHandler... handlers) {
     ...
}

See above .... I think we want to make this more consistent with what we have in the server handler:

https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java#L152
In fact I did try this approach initially and realized I will be breaking existing public constructor of Http2ClientUpgradeCodec. For example, contrary to Http2ServerUpgradeCodec, the Http2ClientUpgradeCodec has a public constructor that allow users to specify a separate ChannelHandler for upgradeTo as shown bleow

```java
public Http2ClientUpgradeCodec(Http2FrameCodec frameCodec, ChannelHandler upgradeToHandler)
```
I havent used this signature personnally - but not sure if other users of netty out there will be leveraging this to pass separate upgradeToHandler other than the frameCodec.

Adding the above proposed approach will introduce ambiguity to upradeToHandler unless we strongly type. 

Let me know your thoughts, if we are okay breaking this constructor - I will go ahead and  make the change consistent with Http2ServerUpgradeCodec
ah yeah you are right. I did not realise it... Then its ok as it is. Just please add an extra test case to `Http2ClientUpgradeCodecTest`.
Missing space after if

Patch that resolved the issue:
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 6028a6fcd15..43c6d5dddc6 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 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,24 +87,38 @@ 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 = checkNotNull(connectionHandler, "connectionHandler");
         this.upgradeToHandler = checkNotNull(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;
@@ -99,12 +126,24 @@ public Collection<CharSequence> setUpgradeHeaders(ChannelHandlerContext ctx,
 
     @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();
+        }
     }
 
     /**


Test patch:
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 2d381e4f88b..f0b4a581c02 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
@@ -33,26 +33,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 ChannelInboundHandlerAdapter()).build());
+            .withUpgradeStreamHandler(new ChannelInboundHandlerAdapter()).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 ChannelInboundHandlerAdapter());
         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();
@@ -60,6 +76,10 @@ private static void testUpgrade(Http2ConnectionHandler handler) throws Exception
         codec.upgradeTo(ctx, null);
         assertNotNull(channel.pipeline().get("connectionHandler"));
 
+        if (multiplexer != null) {
+            assertNotNull(channel.pipeline().get(Http2MultiplexHandler.class));
+        }
+
         assertTrue(channel.finishAndReleaseAll());
     }