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.
This commit is contained in:
zhangduo 2015-07-07 10:50:02 +08:00 committed by nmittler
parent cdbf43596d
commit e949dcd94f
4 changed files with 20 additions and 11 deletions

View File

@ -249,9 +249,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
throw e; throw e;
} finally { } finally {
// If appropriate, return the processed bytes to the flow controller. // 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) { if (endOfStream) {
lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture()); lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture());

View File

@ -142,16 +142,18 @@ public class DefaultHttp2LocalFlowController implements Http2LocalFlowController
@Override @Override
public boolean consumeBytes(ChannelHandlerContext ctx, Http2Stream stream, int numBytes) public boolean consumeBytes(ChannelHandlerContext ctx, Http2Stream stream, int numBytes)
throws Http2Exception { 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 // Streams automatically consume all remaining bytes when they are closed, so just ignore
// if already closed. // if already closed.
if (stream != null && !isClosed(stream)) { if (stream != null && !isClosed(stream)) {
if (stream.id() == CONNECTION_STREAM_ID) { if (stream.id() == CONNECTION_STREAM_ID) {
throw new UnsupportedOperationException("Returning bytes for the connection window is not supported"); 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); boolean windowUpdateSent = connectionState().consumeBytes(ctx, numBytes);
windowUpdateSent |= state(stream).consumeBytes(ctx, numBytes); windowUpdateSent |= state(stream).consumeBytes(ctx, numBytes);
return windowUpdateSent; return windowUpdateSent;

View File

@ -253,14 +253,13 @@ public class DefaultHttp2ConnectionDecoderTest {
public void emptyDataFrameShouldApplyFlowControl() throws Exception { public void emptyDataFrameShouldApplyFlowControl() throws Exception {
final ByteBuf data = EMPTY_BUFFER; final ByteBuf data = EMPTY_BUFFER;
int padding = 0; int padding = 0;
int processedBytes = data.readableBytes() + padding; mockFlowControl(0);
mockFlowControl(processedBytes);
try { try {
decode().onDataRead(ctx, STREAM_ID, data, padding, true); decode().onDataRead(ctx, STREAM_ID, data, padding, true);
verify(localFlow).receiveFlowControlledFrame(eq(ctx), eq(stream), eq(data), eq(padding), eq(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. // Now we ignore the empty bytes inside consumeBytes method, so it will be called once.
verify(localFlow, never()).consumeBytes(eq(ctx), eq(stream), eq(processedBytes)); verify(localFlow).consumeBytes(eq(ctx), eq(stream), eq(0));
// Verify that the empty data event was propagated to the observer. // 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)); verify(listener).onDataRead(eq(ctx), eq(STREAM_ID), eq(data), eq(padding), eq(true));

View File

@ -247,6 +247,16 @@ public class DefaultHttp2LocalFlowControllerTest {
testRatio(ratio, DEFAULT_WINDOW_SIZE << 1, 3, true); 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) private void testRatio(float ratio, int newDefaultWindowSize, int newStreamId, boolean setStreamRatio)
throws Http2Exception { throws Http2Exception {
int delta = newDefaultWindowSize - DEFAULT_WINDOW_SIZE; int delta = newDefaultWindowSize - DEFAULT_WINDOW_SIZE;