Explict always call ctx.read() when AUTO_READ is false and HTTP/2 is …

…used. (netty#8647)

Motivation:

We should always call ctx.read() even when AUTO_READ is false as flow-control is enforced by the HTTP/2 protocol.

See also https://tools.ietf.org/html/rfc7540#section-5.2.2.

We already did this before but not explicit and only did so because of some implementation details of ByteToMessageDecoder. It's better to be explicit here to not risk of breakage later on.

Modifications:

- Ensure we always call ctx.read() when AUTO_READ is false
- Add unit test.

Result:

No risk of staling the connection when HTTP/2 is used.

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

@@ -527,8 +527,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws Exception {
         }
     }
 
-    void channelReadComplete0(ChannelHandlerContext ctx) throws Exception {
-        super.channelReadComplete(ctx);
+    final void channelReadComplete0(ChannelHandlerContext ctx) {
+        // Discard bytes of the cumulation buffer if needed.
+        discardSomeReadBytes();
+
+        // Ensure we never stale the HTTP/2 Channel. Flow-control is enforced by HTTP/2.
+        //
+        // See https://tools.ietf.org/html/rfc7540#section-5.2.2
+        if (!ctx.channel().config().isAutoRead()) {
+            ctx.read();
+        }
+
+        ctx.fireChannelReadComplete();
     }
 
     /**

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java

@@ -22,8 +22,10 @@
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelMetadata;
 import io.netty.channel.ChannelPipeline;
 import io.netty.channel.ChannelPromise;
+import io.netty.channel.DefaultChannelConfig;
 import io.netty.channel.DefaultChannelPromise;
 import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http2.Http2CodecUtil.SimpleChannelPromiseAggregator;
@@ -142,6 +144,11 @@ public void setup() throws Exception {
 
         promise = new DefaultChannelPromise(channel, ImmediateEventExecutor.INSTANCE);
         voidPromise = new DefaultChannelPromise(channel, ImmediateEventExecutor.INSTANCE);
+
+        when(channel.metadata()).thenReturn(new ChannelMetadata(false));
+        DefaultChannelConfig config = new DefaultChannelConfig(channel);
+        when(channel.config()).thenReturn(config);
+
         Throwable fakeException = new RuntimeException("Fake exception");
         when(encoder.connection()).thenReturn(connection);
         when(decoder.connection()).thenReturn(connection);
@@ -684,6 +691,14 @@ public void channelReadCompleteTriggersFlush() throws Exception {
         verify(ctx, times(1)).flush();
     }
 
+    @Test
+    public void channelReadCompleteCallsReadWhenAutoReadFalse() throws Exception {
+        channel.config().setAutoRead(false);
+        handler = newHandler();
+        handler.channelReadComplete(ctx);
+        verify(ctx, times(1)).read();
+    }
+
     @Test
     public void channelClosedDoesNotThrowPrefaceException() throws Exception {
         when(connection.isServer()).thenReturn(true);