From 123e07ca80bb9cc58ebee598ff0a9379ad9a09fd Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 17 Aug 2017 19:00:19 +0200 Subject: [PATCH] Revert "Only call ctx.fireChannelReadComplete() if ByteToMessageDecoder decoded at least one message." This reverts commit d63bb4811ed8ccd5d9e45853f3ac6aee9da7ecab as this not covered correctly all cases and so could lead to missing fireChannelReadComplete() calls. We will re-evalute d63bb4811ed8ccd5d9e45853f3ac6aee9da7ecab and resbumit a pr once we are sure all is handled correctly --- .../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, 18 insertions(+), 58 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 e6ffa0f03a..057c7507a1 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,10 +125,13 @@ public class SpdyFrameCodec extends ByteToMessageDecoder @Override public void channelReadComplete(ChannelHandlerContext ctx) throws Exception { - boolean wasRead = read; + if (!read) { + if (!ctx.channel().config().isAutoRead()) { + ctx.read(); + } + } read = false; - - channelReadComplete(ctx, wasRead); + super.channelReadComplete(ctx); } @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 7f32ae5024..28c3dc12f4 100644 --- a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java +++ b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java @@ -313,18 +313,15 @@ 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(); - decodeWasNull = false; - if (readData) { - ctx.fireChannelReadComplete(); - } else if (!ctx.channel().config().isAutoRead()) { - ctx.read(); + if (decodeWasNull) { + decodeWasNull = false; + 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 84a591611f..8462ee4a02 100644 --- a/codec/src/test/java/io/netty/handler/codec/ByteToMessageDecoderTest.java +++ b/codec/src/test/java/io/netty/handler/codec/ByteToMessageDecoderTest.java @@ -26,8 +26,6 @@ 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; @@ -307,41 +305,4 @@ 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 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 8d0f89b4d8..b03417d15e 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -1167,15 +1167,14 @@ 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); - boolean readData = firedChannelRead; + readIfNeeded(ctx); + firedChannelRead = false; - // 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); + ctx.fireChannelReadComplete(); } private void readIfNeeded(ChannelHandlerContext ctx) {