Repository: netty/netty
Issue: No way to get a Http2StreamChannel for a client h2c upgrade stream via Http2MultiplexCodec

### Expected behavior
When using the `Http2MultiplexCodec` on a client after a h2c upgrade is performed, I expect to be able to get a handle on the `Http2StreamChannel` associated with the upgrade request when using the `Http2MultiplexCodec`.

### Actual behavior
There is no public API for surfacing a `Http2StreamChannel` that resulted from the client request.

### Netty version
Branch 4.1 (as of SHA 0bce0450c05697d)

Issue comments:
There is a solid chance I'm just missing something, and if so, sorry for the noise.
@bryce-anderson basically you are asking to get access to stream 1 (upgrade stream) ?
@normanmaurer, yes, specifically as a client since I wasn't clear on that.
@bryce-anderson I think you are right (but I am a bit busy atm so I may miss something). Did you check `Http2FrameCodec`? If its not possible atm are you interested in doing a PR ?
@normanmaurer, I think the implementation of `Http2StreamChannel` in the private class `DefaultHttp2StreamChannel` inside the `Http2MultiplexCodec` and I don't see a way to surface it (outside of reflection magic).

I'm more than happy to work on a PR presuming @ejona86 or someone else more versed doesn't point me to an API I missed. I'll start on it tomorrow unless someone intervenes. 
What do you want the `Http2StreamChannel` for? It seems the only use is to get the `Http2StreamChannel`. IIRC, that should not be used when using `Http2MultiplexCodec`; Http2FrameStream is only for when using `Http2FrameCodec` directly.
@ejona86, I'm not sure what you mean. I don't want to use the `Http2FrameStream`, I want to use the child channels in the `Http2MultiplexCodec`, and I think they are all implementations of the `Http2StreamChannel`, which is what you get from the `Http2StreamChannelBootstrap`. Unfortunately you can't get the `Http2StreamChannel` for the upgrade stream since it was created for you by the upgrade mechanism.
You can't get _any_ of the channels created for you. That's true for any inbound request. Instead, you can specify the handler that will be installed in the pipeline for that channel. If you need to interact with that channel, then you coordinate with your handler. That's standard Netty for incoming connections.

What am I missing?
I'm talking about a client side usage of `Http2MultiplexCodec` when doing a h2c upgrade. How should I be using it for the request that resulted in the upgrade? In a way, I did create that channel via my HTTP/1.x upgrade request, but it is not like streams 3+ where it begins as a h2 stream from birth.
Ah, I see. _Client-side._ And you did say client in the issue title.

I only did server-side for this before. Client side looks more complicated API-wise, because we don't know whether we'll be speaking HTTP/1 or HTTP/2. The current upgrade API is call-back based, where you provide a Handler that will be used when appropriate.

It's a bit hard for me to see how this all works, because the current example relies on using HTTP/2 API and then using HttpToHttp2ConnectionHandler or Http2ConnectionHandler depending on the upgrade result. Although, maybe that's what we need here, use a HTTP/2 handler with HTTP/1 conversion logic and if the upgrade is successful the handler would be "migrated" to a child channel (although add/removal from the pipeline may be confusing).

Alternatively, maybe the handler could be a HTTP/1 handler that continues receiving HTTP/1 frames (converted from HTTP/2) until the end of the response, and it just ignores any HTTP/2 frames flowing past. In that solution, there _wouldn't_ be a childchan for the upgraded stream. (Note: the handler could actually speak HTTP/2, but there would need to be another HTTP/1 to HTTP/2 conversion; yes, that means we would convert HTTP/2 to HTTP/1 to HTTP/2 for the upgraded stream.)

Yeah. This would take some time to figure out.
Sorry for the confusion about using this client side.

An awkward part of the 'flow-past' solution is that you can't manage response backpressure for the upgraded stream in the normal Netty manner since if you turn off auto-read or you stall the whole h2 session.

My first inclination is to mange this via a richer event model where if the Http2MultiplexCodec gets installed due to a h2c upgrade, the tail of the pipeline gets an event that provides a handle for the child stream and it's the tail of the pipelines job to make the transition. The transition will be pretty complex, but h2c upgrades are anything but simple.
For Finagles current usage, since we're not doing the dispatch work in the pipeline, it's relatively easy for us to switch the underlying `Channel`, but this isn't exactly a normal pattern when using Netty directly.

I'm happy to start putting together something along those lines if it sounds like a reasonable direction to try out.

Ah... flow control. Of course. So axe the HTTP/1 conversion idea.

> the tail of the pipeline gets an event that provides a handle for the child stream and it's the tail of the pipelines job to make the transition

This is sort of fine, but it is timing sensitive ("racy"). If any handler in the pipeline delayed that event, then frames could begin appearing in the childchan before the transition has started.

I think simply passing in a "stream-1 HTTP/2 handler" to use if upgrade succeeds would be more straight-forward, and more in line with the rest of the upgrade logic. Complexity-wise it seems similar or maybe easier to use (although there may be a cost to preparing that handler even when unused).
@ejona86, you're right, the racy nature of pushing messages down that new child stream channel could be tricky and it's not really the same pattern used elsewhere. I like the idea of providing an upgrade initializer/handler.

I'd like to get this sorted out sooner than later so we can start using the `Http2MultiplexCodec` in our client, so I'm happy to take a run at this unless someone else feels like they really want to tackle it.
@bryce-anderson, go for it. I couldn't work on it soon and the upgrade handler seems good enough to me.
👍 sounds good to me

> Am 17.05.2018 um 18:16 schrieb Eric Anderson <notifications@github.com>:
> 
> @bryce-anderson, go for it. I couldn't work on it soon and the upgrade handler seems good enough to me.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 


Pull request review comments:
nit: remove ?
you can remove the `= null`
please enclose with `{}`
nit: remove
You could also just use `ChannelInboundHandlerAdapter` and so not need to declare an extra class .
2018
formatting 
nit: final
private ?
s/he/the/
can we come up with a better name than stream2?
ffti: s/utilize/use
Ha, it seems I'm not very creative. I cargo culted the name from the Http2FrameCodec.onStreamActive0 method, but should have changed the name. Good catch. I'll change it to `codecStream`.

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
index da2b27a1d7b..47f2533f3f8 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
@@ -143,7 +143,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
 
     private static final InternalLogger LOG = InternalLoggerFactory.getInstance(Http2FrameCodec.class);
 
-    private final PropertyKey streamKey;
+    protected final PropertyKey streamKey;
     private final PropertyKey upgradeKey;
 
     private final Integer initialFlowControlWindowSize;
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java
index 515b21d495d..5f17c7503f9 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java
@@ -49,7 +49,11 @@
 import java.util.ArrayDeque;
 import java.util.Queue;
 
+import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_STREAM_ID;
 import static io.netty.handler.codec.http2.Http2CodecUtil.isStreamIdValid;
+import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR;
+import static io.netty.handler.codec.http2.Http2Exception.connectionError;
+
 import static java.lang.Math.min;
 
 /**
@@ -153,6 +157,7 @@ public int guess() {
     }
 
     private final ChannelHandler inboundStreamHandler;
+    private final ChannelHandler upgradeStreamHandler;
 
     private int initialOutboundStreamWindow = Http2CodecUtil.DEFAULT_WINDOW_SIZE;
     private boolean parentReadInProgress;
@@ -168,9 +173,25 @@ public int guess() {
     Http2MultiplexCodec(Http2ConnectionEncoder encoder,
                         Http2ConnectionDecoder decoder,
                         Http2Settings initialSettings,
-                        ChannelHandler inboundStreamHandler) {
+                        ChannelHandler inboundStreamHandler,
+                        ChannelHandler upgradeStreamHandler) {
         super(encoder, decoder, initialSettings);
         this.inboundStreamHandler = inboundStreamHandler;
+        this.upgradeStreamHandler = upgradeStreamHandler;
+    }
+
+    @Override
+    public void onHttpClientUpgrade() throws Http2Exception {
+        // We must have an upgrade handler or else we can't handle the stream
+        if (upgradeStreamHandler == null) {
+            throw connectionError(INTERNAL_ERROR, "Client is misconfigured for upgrade requests");
+        }
+        // Creates the Http2Stream in the Connection.
+        super.onHttpClientUpgrade();
+        // Now make a new FrameStream, set it's underlying Http2Stream, and initialize it.
+        Http2MultiplexCodecStream codecStream = newStream();
+        codecStream.setStreamAndProperty(streamKey, connection().stream(HTTP_UPGRADE_STREAM_ID));
+        onHttp2UpgradeStreamInitialized(ctx, codecStream);
     }
 
     private static void registerDone(ChannelFuture future) {
@@ -236,6 +257,22 @@ final void onHttp2Frame(ChannelHandlerContext ctx, Http2Frame frame) {
         }
     }
 
+    private void onHttp2UpgradeStreamInitialized(ChannelHandlerContext ctx, Http2MultiplexCodecStream stream) {
+        assert stream.state() == Http2Stream.State.HALF_CLOSED_LOCAL;
+        DefaultHttp2StreamChannel ch = new DefaultHttp2StreamChannel(stream, true);
+        ch.outboundClosed = true;
+
+        // Add our upgrade handler to the channel and then register the channel.
+        // The register call fires the channelActive, etc.
+        ch.pipeline().addLast(upgradeStreamHandler);
+        ChannelFuture future = ctx.channel().eventLoop().register(ch);
+        if (future.isDone()) {
+            registerDone(future);
+        } else {
+            future.addListener(CHILD_CHANNEL_REGISTRATION_LISTENER);
+        }
+    }
+
     @Override
     final void onHttp2StreamStateChanged(ChannelHandlerContext ctx, Http2FrameStream stream) {
         Http2MultiplexCodecStream s = (Http2MultiplexCodecStream) stream;
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java
index 8e0929094a6..94f0d497203 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java
@@ -29,6 +29,7 @@ public class Http2MultiplexCodecBuilder
         extends AbstractHttp2ConnectionHandlerBuilder<Http2MultiplexCodec, Http2MultiplexCodecBuilder> {
 
     final ChannelHandler childHandler;
+    private ChannelHandler upgradeStreamHandler;
 
     Http2MultiplexCodecBuilder(boolean server, ChannelHandler childHandler) {
         server(server);
@@ -83,6 +84,14 @@ public Http2MultiplexCodecBuilder gracefulShutdownTimeoutMillis(long gracefulShu
         return super.gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis);
     }
 
+    public Http2MultiplexCodecBuilder withUpgradeStreamHandler(ChannelHandler upgradeStreamHandler) {
+        if (this.isServer()) {
+            throw new IllegalArgumentException("Server codecs don't use an extra handler for the upgrade stream");
+        }
+        this.upgradeStreamHandler = upgradeStreamHandler;
+        return this;
+    }
+
     @Override
     public boolean isServer() {
         return super.isServer();
@@ -157,6 +166,6 @@ public Http2MultiplexCodec build() {
     @Override
     protected Http2MultiplexCodec build(
             Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder, Http2Settings initialSettings) {
-        return new Http2MultiplexCodec(encoder, decoder, initialSettings, childHandler);
+        return new Http2MultiplexCodec(encoder, decoder, initialSettings, childHandler, upgradeStreamHandler);
     }
 }


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 cb774a94ca9..fcfdb4b75e5 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
@@ -14,9 +14,11 @@
  */
 package io.netty.handler.codec.http2;
 
+import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.channel.ChannelInitializer;
 import io.netty.channel.embedded.EmbeddedChannel;
 import io.netty.handler.codec.http.DefaultFullHttpRequest;
 import io.netty.handler.codec.http.FullHttpRequest;
@@ -43,7 +45,8 @@ public void testUpgradeToHttp2FrameCodec() throws Exception {
 
     @Test
     public void testUpgradeToHttp2MultiplexCodec() throws Exception {
-        testUpgrade(Http2MultiplexCodecBuilder.forClient(new HttpInboundHandler()).build());
+        testUpgrade(Http2MultiplexCodecBuilder.forClient(new HttpInboundHandler())
+            .withUpgradeStreamHandler(new ChannelInboundHandlerAdapter()).build());
     }
 
     private static void testUpgrade(Http2ConnectionHandler handler) throws Exception {
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecClientUpgradeTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecClientUpgradeTest.java
new file mode 100644
index 00000000000..26b63ed7f9c
--- /dev/null
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecClientUpgradeTest.java
@@ -0,0 +1,81 @@
+/*
+ * Copyright 2018 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License, version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package io.netty.handler.codec.http2;
+
+import org.junit.Test;
+
+import io.netty.channel.ChannelHandler;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.channel.embedded.EmbeddedChannel;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class Http2MultiplexCodecClientUpgradeTest {
+
+    @ChannelHandler.Sharable
+    private final class NoopHandler extends ChannelInboundHandlerAdapter {
+        @Override
+        public void channelActive(ChannelHandlerContext ctx) {
+            ctx.channel().close();
+        }
+    }
+
+    private final class UpgradeHandler extends ChannelInboundHandlerAdapter {
+        Http2Stream.State stateOnActive;
+        int streamId;
+
+        @Override
+        public void channelActive(ChannelHandlerContext ctx) throws Exception {
+            Http2StreamChannel ch = (Http2StreamChannel) ctx.channel();
+            stateOnActive = ch.stream().state();
+            streamId = ch.stream().id();
+            super.channelActive(ctx);
+        }
+    }
+
+    private Http2MultiplexCodec newCodec(ChannelHandler upgradeHandler) {
+        Http2MultiplexCodecBuilder builder = Http2MultiplexCodecBuilder.forClient(new NoopHandler());
+        builder.withUpgradeStreamHandler(upgradeHandler);
+        return builder.build();
+    }
+
+    @Test
+    public void upgradeHandlerGetsActivated() throws Exception {
+        UpgradeHandler upgradeHandler = new UpgradeHandler();
+        Http2MultiplexCodec codec = newCodec(upgradeHandler);
+        EmbeddedChannel ch = new EmbeddedChannel(codec);
+
+        codec.onHttpClientUpgrade();
+
+        assertFalse(upgradeHandler.stateOnActive.localSideOpen());
+        assertTrue(upgradeHandler.stateOnActive.remoteSideOpen());
+        assertEquals(1, upgradeHandler.streamId);
+        assertTrue(ch.finishAndReleaseAll());
+    }
+
+    @Test(expected = Http2Exception.class)
+    public void clientUpgradeWithoutUpgradeHandlerThrowsHttp2Exception() throws Http2Exception {
+        Http2MultiplexCodec codec = Http2MultiplexCodecBuilder.forClient(new NoopHandler()).build();
+        EmbeddedChannel ch = new EmbeddedChannel(codec);
+        try {
+            codec.onHttpClientUpgrade();
+        } finally {
+            assertTrue(ch.finishAndReleaseAll());
+        }
+    }
+}
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java
index 7a0e8c6a3a0..2b398ef62cc 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java
@@ -633,7 +633,7 @@ public TestableHttp2MultiplexCodec(Http2ConnectionEncoder encoder,
                                            Http2ConnectionDecoder decoder,
                                            Http2Settings initialSettings,
                                            ChannelHandler inboundStreamHandler) {
-            super(encoder, decoder, initialSettings, inboundStreamHandler);
+            super(encoder, decoder, initialSettings, inboundStreamHandler, null);
         }
 
         void onHttp2Frame(Http2Frame frame) {