Repository: netty/netty
Issue: h2childchan: DefaultHttp2StreamChannel initial writability

Currently the initial writability of `DefaultHttp2StreamChannel` is false if we have not yet created the underlying `Http2Stream` object. Typically a channel is default writability is true, unless conditions otherwise prohibit this. In the case of `DefaultHttp2StreamChannel` we should investigate if the following policy makes sense: the default state is writable unless the connection window has collapsed, and we may also want to consider the parent channel's writability.

https://github.com/netty/netty/pull/6896#discussion_r132339750

Issue comments:
This is "fixed" by https://github.com/netty/netty/pull/7181 ... To make it short no it makes no sense.
Fixed by #7181 
@normanmaurer - Can you explain why the policy suggested here "makes no sense"? The policy implemented in #7181 seems incomplete and may incorrectly report writability in a few cases:
- depending on initial settings for outbound streams (https://github.com/netty/netty/pull/7181#discussion_r139296524)
- depending on connection window
- depending on channel writability

Pull request review comments:
false positive ?
I'm not seeing a `setStreamAndProperty()` for client-created streams... Should it be part of `Http2FrameCodec.writeHeadersFrame()`?

If `s == null` then the stream must be client-side, since newStream() on server-side is initialized immediately. Since the headers frame/first frame issue is handled elsewhere, this probably should never be called with `s == null` (other than the issue that it looks like `s` is always null on client-side).
s/evulate/evaluate/
for client side its triggered by this code path:

https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java#L397


fixed
Oh, I see now. I don't feel bad missing that slight-of-hand. That does look like it will break when hitting MAX_CONCURRENT_STREAMS since StreamBufferingEncoder won't create the stream immediately.

So if `s == null` here, then things are weird/broken elsewhere, so it shouldn't really matter whether we consider it writable or not.
This may not be correct in all cases. The peer can send a settings frame with initial_window_size of 0 which would mean new streams are not writable. A server/client may do this if they are being overwhelmed and/or was overly optimistic about how many resources it initially advertised ... not sure how common it is in practice.

We should consider checking the peer's initial window size if this is a locally created stream.
can we rename `writabilityChanged` to `checkForWritabilityChange` (or something similar)? At this point we are not indicating there is a writability change ... we are just checking if there has been a change.
I guess we currently don't have a test which closes a channel after it has written to fast, to much, or the peer stops reading? Seems like a potentially "flaky" check to have here ... if conditions/tests change ... can you add a comment?
The writability needs to be true so the application provides the initial headers. After we have the headers the stream exists and this won't be used any more. So the application shouldn't end up writing any DATA.
I think what @ejona86 said is correct and so it should be correct @Scottmitch 
Ahh I see ... yes this makes sense. Although we don't make the same consideration for trailers. We don't really know when the trailers will be written so it is possible the user will write data until not writable and then want to write trailers but if they are using channel writability to conditionally write then they will have to wait. Writing trailers maybe less common, but it seems strange to exhibit different behavior in these scenarios. 
This has nothing to do with HEADERS vs DATA and how they are impacted by HTTP/2 flow control. It has everything to do with "the stream doesn't exist before the initial headers." So I don't really see the trailers as being weird at all. If client could send multiple headers (say, like server can with 1xx codes) then we'd still want this same logic.
Yip understood this is to facilitate stream creation.

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 b1bcdd213a0..a0685899632 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
@@ -582,6 +582,11 @@ void onHttp2FrameStreamException(ChannelHandlerContext ctx, Http2FrameStreamExce
         ctx.fireExceptionCaught(cause);
     }
 
+    final boolean isWritable(DefaultHttp2FrameStream stream) {
+        Http2Stream s = stream.stream;
+        return s != null && connection().remote().flowController().isWritable(s);
+    }
+
     private final class Http2RemoteFlowControllerListener implements Http2RemoteFlowController.Listener {
         @Override
         public void writabilityChanged(Http2Stream stream) {
@@ -601,7 +606,7 @@ public void writabilityChanged(Http2Stream stream) {
     static class DefaultHttp2FrameStream implements Http2FrameStream {
 
         private volatile int id = -1;
-        private volatile Http2Stream stream;
+        volatile Http2Stream stream;
 
         DefaultHttp2FrameStream setStreamAndProperty(PropertyKey streamKey, Http2Stream stream) {
             assert id == -1 || stream.id() == id;
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 db513d3dae8..373bb9bd489 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
@@ -231,16 +231,17 @@ final void onHttp2Frame(ChannelHandlerContext ctx, Http2Frame frame) {
 
     @Override
     final void onHttp2StreamStateChanged(ChannelHandlerContext ctx, Http2FrameStream stream) {
+        Http2MultiplexCodecStream s = (Http2MultiplexCodecStream) stream;
+
         switch (stream.state()) {
             case HALF_CLOSED_REMOTE:
             case OPEN:
-                if (((Http2MultiplexCodecStream) stream).channel != null) {
+                if (s.channel != null) {
                     // ignore if child channel was already created.
                     break;
                 }
                 // fall-trough
-                DefaultHttp2StreamChannel childChannel = new DefaultHttp2StreamChannel(stream, false);
-                ChannelFuture future = ctx.channel().eventLoop().register(childChannel);
+                ChannelFuture future = ctx.channel().eventLoop().register(new DefaultHttp2StreamChannel(s, false));
                 if (future.isDone()) {
                     registerDone(future);
                 } else {
@@ -248,7 +249,7 @@ final void onHttp2StreamStateChanged(ChannelHandlerContext ctx, Http2FrameStream
                 }
                 break;
             case CLOSED:
-                DefaultHttp2StreamChannel channel = ((Http2MultiplexCodecStream) stream).channel;
+                DefaultHttp2StreamChannel channel = s.channel;
                 if (channel != null) {
                     channel.streamClosed();
                 }
@@ -395,17 +396,25 @@ private enum ReadState {
         READ_PROCESSED_OK_TO_PROCESS_MORE
     }
 
+    private boolean initialWritability(DefaultHttp2FrameStream stream) {
+        // If the stream id is not valid yet we will just mark the channel as writable as we will be notified
+        // about non-writability state as soon as the first Http2HeaderFrame is written (if needed).
+        // This should be good enough and simplify things a lot.
+        return !isStreamIdValid(stream.id()) || isWritable(stream);
+    }
+
     // TODO: Handle writability changes due writing from outside the eventloop.
     private final class DefaultHttp2StreamChannel extends DefaultAttributeMap implements Http2StreamChannel {
         private final Http2StreamChannelConfig config = new Http2StreamChannelConfig(this);
         private final Http2ChannelUnsafe unsafe = new Http2ChannelUnsafe();
         private final ChannelId channelId;
         private final ChannelPipeline pipeline;
-        private final Http2FrameStream stream;
+        private final DefaultHttp2FrameStream stream;
         private final ChannelPromise closePromise;
         private final boolean outbound;
 
         private volatile boolean registered;
+        // We start with the writability of the channel when creating the StreamChannel.
         private volatile boolean writable;
 
         private boolean closePending;
@@ -428,9 +437,10 @@ private final class DefaultHttp2StreamChannel extends DefaultAttributeMap implem
         // channelReadComplete(...)
         DefaultHttp2StreamChannel next;
 
-        DefaultHttp2StreamChannel(Http2FrameStream stream, boolean outbound) {
+        DefaultHttp2StreamChannel(DefaultHttp2FrameStream stream, boolean outbound) {
             this.stream = stream;
             this.outbound = outbound;
+            writable = initialWritability(stream);
             ((Http2MultiplexCodecStream) stream).channel = this;
             pipeline = new DefaultChannelPipeline(this) {
                 @Override
@@ -484,8 +494,7 @@ public boolean isActive() {
 
         @Override
         public boolean isWritable() {
-            // TODO: Should we also take the parent channel into account ?
-            return isStreamIdValid(stream.id()) && writable;
+            return writable;
         }
 
         @Override
@@ -1011,6 +1020,9 @@ public void operationComplete(ChannelFuture future) throws Exception {
             private void firstWriteComplete(ChannelFuture future, ChannelPromise promise) {
                 Throwable cause = future.cause();
                 if (cause == null) {
+                    // As we just finished our first write which made the stream-id valid we need to re-evaluate
+                    // the writability of the channel.
+                    writabilityChanged(Http2MultiplexCodec.this.isWritable(stream));
                     promise.setSuccess();
                 } else {
                     promise.setFailure(cause);


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 f798b021f70..c394689fb6b 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
@@ -24,6 +24,7 @@
 import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandler.Sharable;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.ChannelInitializer;
 import io.netty.channel.DefaultEventLoop;
 import io.netty.channel.EventLoopGroup;
@@ -74,7 +75,34 @@ public void setUp() throws InterruptedException {
                     @Override
                     protected void initChannel(Channel ch) throws Exception {
                         serverConnectedChannel = ch;
-                        ch.pipeline().addLast(new Http2MultiplexCodecBuilder(true, serverLastInboundHandler).build());
+                        ch.pipeline().addLast(new Http2MultiplexCodecBuilder(true, new ChannelInitializer<Channel>() {
+
+                            @Override
+                            protected void initChannel(Channel ch) throws Exception {
+                                ch.pipeline().addLast(new ChannelInboundHandlerAdapter() {
+                                    private boolean writable;
+
+                                    @Override
+                                    public void channelActive(ChannelHandlerContext ctx) throws Exception {
+                                        writable |= ctx.channel().isWritable();
+                                        super.channelActive(ctx);
+                                    }
+
+                                    @Override
+                                    public void channelWritabilityChanged(ChannelHandlerContext ctx) throws Exception {
+                                        writable |= ctx.channel().isWritable();
+                                        super.channelWritabilityChanged(ctx);
+                                    }
+
+                                    @Override
+                                    public void channelInactive(ChannelHandlerContext ctx) throws Exception {
+                                        assertTrue(writable);
+                                        super.channelInactive(ctx);
+                                    }
+                                });
+                                ch.pipeline().addLast(serverLastInboundHandler);
+                            }
+                        }).build());
                         serverChannelLatch.countDown();
                     }
                 });
@@ -120,7 +148,7 @@ private Http2StreamChannel newOutboundStream(ChannelHandler handler) {
     }
 
     @Test
-    public void multipleOutboundStreams() {
+    public void multipleOutboundStreams() throws Exception {
         Http2StreamChannel childChannel1 = newOutboundStream(new TestChannelInitializer());
         assertTrue(childChannel1.isActive());
         assertFalse(isStreamIdValid(childChannel1.stream().id()));
@@ -148,10 +176,12 @@ public void multipleOutboundStreams() {
 
         childChannel1.close();
         childChannel2.close();
+
+        serverLastInboundHandler.checkException();
     }
 
     @Test
-    public void createOutboundStream() {
+    public void createOutboundStream() throws Exception {
         Channel childChannel = newOutboundStream(new TestChannelInitializer());
         assertTrue(childChannel.isRegistered());
         assertTrue(childChannel.isActive());
@@ -178,10 +208,13 @@ public void createOutboundStream() {
         Http2ResetFrame rstFrame = serverLastInboundHandler.blockingReadInbound();
         assertNotNull(rstFrame);
         assertEquals(3, rstFrame.stream().id());
+
+        serverLastInboundHandler.checkException();
     }
 
     @Sharable
     private static class SharableLastInboundHandler extends LastInboundHandler {
+
         @Override
         public void channelActive(ChannelHandlerContext ctx) throws Exception {
             ctx.fireChannelActive();
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 acf33ee0220..e9b477fdfb5 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
@@ -372,7 +372,7 @@ public void outboundFlowControlWritability() {
         Http2StreamChannel childChannel = newOutboundStream();
         assertTrue(childChannel.isActive());
 
-        assertFalse(childChannel.isWritable());
+        assertTrue(childChannel.isWritable());
         childChannel.writeAndFlush(new DefaultHttp2HeadersFrame(new DefaultHttp2Headers()));
         parentChannel.flush();