Repository: netty/netty
Issue: The ChannelHandler which is used to construct Http2MultiplexCodecBuilder will also be added to out bound streams
Labels: defect

### Expected behavior
The `ChannelHandler` which is used to construct `Http2MultiplexCodecBuilder` should only be added to remotely-created streams.
### Actual behavior
It is also added to out bound streams.
### Steps to reproduce
Modify `Http2MultiplexCodecBuilderTest`, replace the code in `setUp`

```
        Bootstrap cb = new Bootstrap()
                .channel(LocalChannel.class)
                .group(group)
                .handler(new Http2MultiplexCodecBuilder(false, new TestChannelInitializer()).build());
```
to
```
        Bootstrap cb = new Bootstrap()
                .channel(LocalChannel.class)
                .group(group)
                .handler(new Http2MultiplexCodecBuilder(false, new ChannelInitializer<Channel>() {

                    @Override
                    protected void initChannel(Channel ch) throws Exception {
                        throw new RuntimeException("should not be call for outbound stream");
                    }
                }).build());
```
Then `createOutboundStream` and `multipleOutboundStreams` will both fail.
### Minimal yet complete reproducer code (or URL to code)
Just apply this patch
https://github.com/Apache9/netty/commit/2dcbae3109bbab5699dbfd64176694c8d43f69ca
### Netty version
4.1.15.Final
### JVM version (e.g. `java -version`)
java version "1.8.0_121"
Java(TM) SE Runtime Environment (build 1.8.0_121-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.121-b13, mixed mode)
### OS version (e.g. `uname -a`)
Linux zhangduo-ubuntu 4.4.0-93-generic #116-Ubuntu SMP Fri Aug 11 21:17:51 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

Issue comments:
The stracktrace of the exception when running modified `Http2MultiplexCodecBuilderTest`.
```
15:49:13.120 [defaultEventLoop-1-1] WARN  io.netty.channel.ChannelInitializer - Failed to initialize a channel. Closing: [id: 0x70bc891d, L:local:E:e07decb9 - R:local:io.netty.handler.codec.http2.http2multiplexcodecbuildertest](HTTP/2 - -1)
java.lang.RuntimeException: should not be call for outbound stream
	at io.netty.handler.codec.http2.Http2MultiplexCodecBuilderTest$2.initChannel(Http2MultiplexCodecBuilderTest.java:89)
	at io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:113)
	at io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:105)
	at io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:606)
	at io.netty.channel.DefaultChannelPipeline.access$15(DefaultChannelPipeline.java:604)
	at io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1412)
	at io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1131)
	at io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:656)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRegistered(DefaultChannelPipeline.java:1331)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRegistered(AbstractChannelHandlerContext.java:149)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRegistered(AbstractChannelHandlerContext.java:135)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRegistered(DefaultChannelPipeline.java:829)
	at io.netty.handler.codec.http2.Http2MultiplexCodec$DefaultHttp2StreamChannel$Http2ChannelUnsafe.register(Http2MultiplexCodec.java:799)
	at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:80)
	at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:74)
	at io.netty.handler.codec.http2.Http2StreamChannelBootstrap.open0(Http2StreamChannelBootstrap.java:143)
	at io.netty.handler.codec.http2.Http2StreamChannelBootstrap$1.run(Http2StreamChannelBootstrap.java:124)
	at io.netty.channel.DefaultEventLoop.run(DefaultEventLoop.java:54)
	at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
	at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:138)
	at java.lang.Thread.run(Thread.java:745)
```
Seems in `Http2MultiplexCodec.DefaultHttp2StreamChannel.Http2ChannelUnsafe.register`
, we always add the `inboundStreamHandler` to the pipeline of the `Http2Stream`, no matter it is inbound or not.

https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java#L795

@normanmaurer 
Yep.. not sure if this is a bug or not tho as you will need to have an handler that handles inbound frames as well here. @ejona86 WDYT?
Yeah we need this handler to handle the inbound streams. But for outboud streams, we will provide one by calling  `Http2StreamChannelBootstrap.handler`. 
I agree that outbound streams should have their channel initializer specified via `Http2StreamChannelBootstrap.handler()`. `Http2MultiplexCodecBuilder`'s handler is only for inbound streams.
Got it... let me do a fix quickly 

> Am 05.09.2017 um 17:10 schrieb Eric Anderson <notifications@github.com>:
> 
> I agree that outbound streams should have their channel initializer specified via Http2StreamChannelBootstrap.handler(). Http2MultiplexCodecBuilder's handler is only for inbound streams.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 

Should be fixed by https://github.com/netty/netty/pull/7180 . Please check
Fixed by https://github.com/netty/netty/pull/7180

Pull request review comments:

Patch that resolved the issue:
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 7cb3ec0c70e..db513d3dae8 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
@@ -239,7 +239,7 @@ final void onHttp2StreamStateChanged(ChannelHandlerContext ctx, Http2FrameStream
                     break;
                 }
                 // fall-trough
-                DefaultHttp2StreamChannel childChannel = new DefaultHttp2StreamChannel(stream);
+                DefaultHttp2StreamChannel childChannel = new DefaultHttp2StreamChannel(stream, false);
                 ChannelFuture future = ctx.channel().eventLoop().register(childChannel);
                 if (future.isDone()) {
                     registerDone(future);
@@ -266,7 +266,7 @@ final void onHttp2StreamWritabilityChanged(ChannelHandlerContext ctx, Http2Frame
 
     // TODO: This is most likely not the best way to expose this, need to think more about it.
     final Http2StreamChannel newOutboundStream() {
-        return new DefaultHttp2StreamChannel(newStream());
+        return new DefaultHttp2StreamChannel(newStream(), true);
     }
 
     @Override
@@ -403,6 +403,7 @@ private final class DefaultHttp2StreamChannel extends DefaultAttributeMap implem
         private final ChannelPipeline pipeline;
         private final Http2FrameStream stream;
         private final ChannelPromise closePromise;
+        private final boolean outbound;
 
         private volatile boolean registered;
         private volatile boolean writable;
@@ -427,8 +428,9 @@ private final class DefaultHttp2StreamChannel extends DefaultAttributeMap implem
         // channelReadComplete(...)
         DefaultHttp2StreamChannel next;
 
-        DefaultHttp2StreamChannel(Http2FrameStream stream) {
+        DefaultHttp2StreamChannel(Http2FrameStream stream, boolean outbound) {
             this.stream = stream;
+            this.outbound = outbound;
             ((Http2MultiplexCodecStream) stream).channel = this;
             pipeline = new DefaultChannelPipeline(this) {
                 @Override
@@ -791,8 +793,10 @@ public void register(EventLoop eventLoop, ChannelPromise promise) {
 
                 registered = true;
 
-                // Add the handler to the pipeline now that we are registered.
-                pipeline().addLast(inboundStreamHandler);
+                if (!outbound) {
+                    // Add the handler to the pipeline now that we are registered.
+                    pipeline().addLast(inboundStreamHandler);
+                }
 
                 promise.setSuccess();
 


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilderTest.java
index 383d17104c9..f798b021f70 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilderTest.java
@@ -32,6 +32,7 @@
 import io.netty.channel.local.LocalServerChannel;
 import org.junit.After;
 import org.junit.AfterClass;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -82,7 +83,12 @@ protected void initChannel(Channel ch) throws Exception {
         Bootstrap cb = new Bootstrap()
                 .channel(LocalChannel.class)
                 .group(group)
-                .handler(new Http2MultiplexCodecBuilder(false, new TestChannelInitializer()).build());
+                .handler(new Http2MultiplexCodecBuilder(false, new ChannelInitializer<Channel>() {
+                    @Override
+                    protected void initChannel(Channel ch) throws Exception {
+                        Assert.fail("Should not be called for outbound streams");
+                    }
+                }).build());
         clientChannel = cb.connect(serverAddress).sync().channel();
         assertTrue(serverChannelLatch.await(5, SECONDS));
     }