From e949dcd94f0cf5ef0e697792c25c79d0b0de3033 Mon Sep 17 00:00:00 2001 From: zhangduo Date: Tue, 7 Jul 2015 10:50:02 +0800 Subject: [PATCH] Allow numBytes == 0 when calling Http2LocalFlowController.consumeBytes. Motivation: Sometimes people use a data frame with length 0 to end a stream(such as jetty http2-server). So it is possible that data.readableBytes and padding are all 0 for a data frame, and cause an IllegalArgumentException when calling flowController.consumeBytes. Modifications: Return false when numBytes == 0 instead of throwing IllegalArgumentException. Result: Fix IllegalArgumentException. --- .../codec/http2/DefaultHttp2ConnectionDecoder.java | 4 +--- .../codec/http2/DefaultHttp2LocalFlowController.java | 10 ++++++---- .../codec/http2/DefaultHttp2ConnectionDecoderTest.java | 7 +++---- .../http2/DefaultHttp2LocalFlowControllerTest.java | 10 ++++++++++ 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java index d477ea889e..8ed41ff2ef 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java @@ -249,9 +249,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { throw e; } finally { // If appropriate, return the processed bytes to the flow controller. - if (bytesToReturn > 0) { - flowController.consumeBytes(ctx, stream, bytesToReturn); - } + flowController.consumeBytes(ctx, stream, bytesToReturn); if (endOfStream) { lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture()); diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowController.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowController.java index 30233bf056..ce60cf48f0 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowController.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowController.java @@ -142,16 +142,18 @@ public class DefaultHttp2LocalFlowController implements Http2LocalFlowController @Override public boolean consumeBytes(ChannelHandlerContext ctx, Http2Stream stream, int numBytes) throws Http2Exception { + if (numBytes < 0) { + throw new IllegalArgumentException("numBytes must not be negative"); + } + if (numBytes == 0) { + return false; + } // Streams automatically consume all remaining bytes when they are closed, so just ignore // if already closed. if (stream != null && !isClosed(stream)) { if (stream.id() == CONNECTION_STREAM_ID) { throw new UnsupportedOperationException("Returning bytes for the connection window is not supported"); } - if (numBytes <= 0) { - throw new IllegalArgumentException("numBytes must be positive"); - } - boolean windowUpdateSent = connectionState().consumeBytes(ctx, numBytes); windowUpdateSent |= state(stream).consumeBytes(ctx, numBytes); return windowUpdateSent; diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java index a56d644024..2aa3e8102d 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java @@ -253,14 +253,13 @@ public class DefaultHttp2ConnectionDecoderTest { public void emptyDataFrameShouldApplyFlowControl() throws Exception { final ByteBuf data = EMPTY_BUFFER; int padding = 0; - int processedBytes = data.readableBytes() + padding; - mockFlowControl(processedBytes); + mockFlowControl(0); try { decode().onDataRead(ctx, STREAM_ID, data, padding, true); verify(localFlow).receiveFlowControlledFrame(eq(ctx), eq(stream), eq(data), eq(padding), eq(true)); - // No bytes were consumed, so there's no window update needed. - verify(localFlow, never()).consumeBytes(eq(ctx), eq(stream), eq(processedBytes)); + // Now we ignore the empty bytes inside consumeBytes method, so it will be called once. + verify(localFlow).consumeBytes(eq(ctx), eq(stream), eq(0)); // Verify that the empty data event was propagated to the observer. verify(listener).onDataRead(eq(ctx), eq(STREAM_ID), eq(data), eq(padding), eq(true)); diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowControllerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowControllerTest.java index 06051a74a8..9cc8a919e2 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowControllerTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowControllerTest.java @@ -247,6 +247,16 @@ public class DefaultHttp2LocalFlowControllerTest { testRatio(ratio, DEFAULT_WINDOW_SIZE << 1, 3, true); } + @Test + public void consumeBytesForZeroNumBytesShouldIgnore() throws Http2Exception { + assertFalse(controller.consumeBytes(ctx, connection.stream(STREAM_ID), 0)); + } + + @Test(expected = IllegalArgumentException.class) + public void consumeBytesForNegativeNumBytesShouldFail() throws Http2Exception { + assertFalse(controller.consumeBytes(ctx, connection.stream(STREAM_ID), -1)); + } + private void testRatio(float ratio, int newDefaultWindowSize, int newStreamId, boolean setStreamRatio) throws Http2Exception { int delta = newDefaultWindowSize - DEFAULT_WINDOW_SIZE;