From d63bb4811ed8ccd5d9e45853f3ac6aee9da7ecab Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Wed, 14 Jun 2017 11:40:25 +0200
Subject: [PATCH] Only call ctx.fireChannelReadComplete() if
 ByteToMessageDecoder decoded at least one message.

Motivation:

Its wasteful and also confusing that channelReadComplete() is called even if there was no message forwarded to the next handler.

Modifications:

- Only call ctx.fireChannelReadComplete() if at least one message was decoded
- Add unit test

Result:

Less confusing behavior. Fixes [#4312].
---
 .../handler/codec/spdy/SpdyFrameCodec.java    |  9 ++---
 .../handler/codec/ByteToMessageDecoder.java   | 15 ++++---
 .../codec/ByteToMessageDecoderTest.java       | 39 +++++++++++++++++++
 .../java/io/netty/handler/ssl/SslHandler.java | 13 ++++---
 4 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameCodec.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameCodec.java
index 057c7507a1..e6ffa0f03a 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameCodec.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameCodec.java
@@ -125,13 +125,10 @@ public class SpdyFrameCodec extends ByteToMessageDecoder
 
     @Override
     public void channelReadComplete(ChannelHandlerContext ctx) throws Exception {
-        if (!read) {
-            if (!ctx.channel().config().isAutoRead()) {
-                ctx.read();
-            }
-        }
+        boolean wasRead = read;
         read = false;
-        super.channelReadComplete(ctx);
+
+        channelReadComplete(ctx, wasRead);
     }
 
     @Override
diff --git a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
index 28c3dc12f4..7f32ae5024 100644
--- a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
@@ -313,15 +313,18 @@ public abstract class ByteToMessageDecoder extends ChannelInboundHandlerAdapter
 
     @Override
     public void channelReadComplete(ChannelHandlerContext ctx) throws Exception {
+        channelReadComplete(ctx, !decodeWasNull);
+    }
+
+    protected final void channelReadComplete(ChannelHandlerContext ctx, boolean readData) throws Exception {
         numReads = 0;
         discardSomeReadBytes();
-        if (decodeWasNull) {
-            decodeWasNull = false;
-            if (!ctx.channel().config().isAutoRead()) {
-                ctx.read();
-            }
+        decodeWasNull = false;
+        if (readData) {
+            ctx.fireChannelReadComplete();
+        } else if (!ctx.channel().config().isAutoRead()) {
+            ctx.read();
         }
-        ctx.fireChannelReadComplete();
     }
 
     protected final void discardSomeReadBytes() {
diff --git a/codec/src/test/java/io/netty/handler/codec/ByteToMessageDecoderTest.java b/codec/src/test/java/io/netty/handler/codec/ByteToMessageDecoderTest.java
index 8462ee4a02..84a591611f 100644
--- a/codec/src/test/java/io/netty/handler/codec/ByteToMessageDecoderTest.java
+++ b/codec/src/test/java/io/netty/handler/codec/ByteToMessageDecoderTest.java
@@ -26,6 +26,8 @@ import org.junit.Test;
 import java.util.List;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.LinkedBlockingDeque;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -305,4 +307,41 @@ public class ByteToMessageDecoderTest {
         assertFalse(channel.writeInbound(Unpooled.wrappedBuffer(new byte[] { (byte) 2 })));
         assertFalse(channel.finish());
     }
+
+    @Test
+    public void testFireChannelReadComplete() {
+        final AtomicBoolean readCompleteExpected = new AtomicBoolean();
+        final AtomicInteger readCompleteCount = new AtomicInteger();
+        EmbeddedChannel channel = new EmbeddedChannel(new ByteToMessageDecoder() {
+
+            @Override
+            protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
+                if (in.readableBytes() > 1) {
+                    readCompleteExpected.set(true);
+                    out.add(in.readBytes(in.readableBytes()));
+                }
+            }
+        }, new ChannelInboundHandlerAdapter() {
+            @Override
+            public void channelReadComplete(ChannelHandlerContext ctx) throws Exception {
+                assertTrue(readCompleteExpected.get());
+                readCompleteCount.incrementAndGet();
+            }
+        });
+
+        assertFalse(channel.writeInbound(Unpooled.wrappedBuffer(new byte[] {'a'})));
+        assertTrue(channel.writeInbound(Unpooled.wrappedBuffer(new byte[] {'b'})));
+        ByteBuf b = channel.readInbound();
+
+        ByteBuf expected = Unpooled.wrappedBuffer(new byte[] {'a', 'b'});
+        assertEquals(expected, b);
+        b.release();
+        expected.release();
+
+        assertTrue(readCompleteExpected.get());
+
+        assertFalse(channel.finish());
+
+        assertEquals(1, readCompleteCount.get());
+    }
 }
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
index b03417d15e..8d0f89b4d8 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -1167,14 +1167,15 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
 
     @Override
     public void channelReadComplete(ChannelHandlerContext ctx) throws Exception {
-        // Discard bytes of the cumulation buffer if needed.
-        discardSomeReadBytes();
-
         flushIfNeeded(ctx);
-        readIfNeeded(ctx);
-
+        boolean readData = firedChannelRead;
         firedChannelRead = false;
-        ctx.fireChannelReadComplete();
+        // if readData is false channelReadComplete(....) will take care of calling read.
+        if (readData && !handshakePromise.isDone() && !ctx.channel().config().isAutoRead()) {
+            // If handshake is not finished yet, we need more data.
+            ctx.read();
+        }
+        channelReadComplete(ctx, readData);
     }
 
     private void readIfNeeded(ChannelHandlerContext ctx) {