Repository: netty/netty
Issue: The Http2MultiplexCodec StreamChannel will fire channelInactive without the channel.isOpen method reporting false
Labels: defect

### Expected behavior

When part of a HTTP/2 `StreamChannel` the `StreamChannel.isOpen()` method should report `false` within a call to a `ChannelInboundHandler`s `channelInactive` method.

### Actual behavior

Within the `channelInactive` method, `ctx.channel().isOpen() == true`

### Minimal yet complete reproducer code (or URL to code)

Within the `Http2MultiplexCodecTest:
```java
    @Test
    public void channelClosedWhenInactiveFired() {
        LastInboundHandler inboundHandler = streamActiveAndWriteHeaders(inboundStream);
        Http2StreamChannel childChannel = (Http2StreamChannel) inboundHandler.channel();

        final AtomicBoolean channelWasOpen = new AtomicBoolean(false);

        childChannel.pipeline().addLast(new ChannelInboundHandlerAdapter() {
            @Override
            public void channelInactive(ChannelHandlerContext ctx) throws Exception {
                channelWasOpen.set(ctx.channel().isOpen());
                super.channelInactive(ctx);
            }
        });

        childChannel.close().syncUninterruptibly();
        assertFalse(channelWasOpen.get());
    }
```

Result: 
java.lang.AssertionError
io.netty.handler.codec.http2.Http2MultiplexCodecTest.channelClosedWhenInactiveFired(Http2MultiplexCodecTest.java:471)

### Netty version

4.1

### JVM version (e.g. `java -version`)
openjdk version "1.8.0_152"

### OS version (e.g. `uname -a`)
Darwin

Issue comments:
See the discussion in https://github.com/netty/netty/pull/7631 for more details including a possible solution.
@bryce-anderson thanks a lot! I will fix it :)
@bryce-anderson PTAL https://github.com/netty/netty/pull/7640
Fixed

Pull request review comments:
I also think the order of these two needs to be reversed, the following test fails:
```java
@Test
    public void channelClosedWhenClosePromiseCompletes() {
        LastInboundHandler inboundHandler = streamActiveAndWriteHeaders(inboundStream);
        Http2StreamChannel childChannel = (Http2StreamChannel) inboundHandler.channel();

        assertTrue(childChannel.isOpen());
        assertTrue(childChannel.isActive());

        final AtomicBoolean channelOpen = new AtomicBoolean(true);

        final ChannelPromise p = childChannel.newPromise();

        p.addListener(new ChannelFutureListener() {
            @Override
            public void operationComplete(ChannelFuture future) throws Exception {
                channelOpen.set(future.channel().isOpen());
            }
        });

        childChannel.close(p).syncUninterruptibly();

        assertFalse(channelOpen.get());
    }
```

Reversing the order fixes the test above. I'm happy to file another bug if you prefer (perhaps that breaks something else I'm not aware of).
+1 ... there was actually another issue too so I just submitted https://github.com/netty/netty/pull/7643 with a modified version of the unit test above.

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 aeeec3feb68..4972a044d59 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
@@ -886,12 +886,14 @@ public void close(ChannelPromise promise) {
                         }
                     }
 
+                    // The promise should be notified before we call fireChannelInactive().
+                    promise.setSuccess();
+                    closePromise.setSuccess();
+
                     pipeline().fireChannelInactive();
                     if (isRegistered()) {
                         deregister(unsafe().voidPromise());
                     }
-                    promise.setSuccess();
-                    closePromise.setSuccess();
                 } finally {
                     pendingClosePromise = null;
                 }


Test patch:
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 bec5cd964ee..4778729cd97 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
@@ -31,6 +31,7 @@
 import io.netty.util.AttributeKey;
 
 import java.net.InetSocketAddress;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.junit.After;
 import org.junit.Before;
@@ -420,6 +421,31 @@ public void writabilityAndFlowControl() {
         assertEquals("true,false", inboundHandler.writabilityStates());
     }
 
+    @Test
+    public void channelClosedWhenInactiveFired() {
+        LastInboundHandler inboundHandler = streamActiveAndWriteHeaders(inboundStream);
+        Http2StreamChannel childChannel = (Http2StreamChannel) inboundHandler.channel();
+
+        final AtomicBoolean channelOpen = new AtomicBoolean(false);
+        final AtomicBoolean channelActive = new AtomicBoolean(false);
+        assertTrue(childChannel.isOpen());
+        assertTrue(childChannel.isActive());
+
+        childChannel.pipeline().addLast(new ChannelInboundHandlerAdapter() {
+            @Override
+            public void channelInactive(ChannelHandlerContext ctx) throws Exception {
+                channelOpen.set(ctx.channel().isOpen());
+                channelActive.set(ctx.channel().isActive());
+
+                super.channelInactive(ctx);
+            }
+        });
+
+        childChannel.close().syncUninterruptibly();
+        assertFalse(channelOpen.get());
+        assertFalse(channelActive.get());
+    }
+
     @Ignore("not supported anymore atm")
     @Test
     public void cancellingWritesBeforeFlush() {