Zero length data frames should apply flow control.
Motivation: A downstream consumer of Netty failed as emitting zero-length http2 data frames in a unit test resulted in assertion errors in Http2LocalFlowController. Since zero-length frames are valid, an assertion that http2 data frame length must be positive is invalid. Modifications: Assertions of data length in Http2LocalFlowController now permit zero. Result: Those running netty with assertions on can now emit zero length http2 data frames.
This commit is contained in:
parent
2d24e1f27d
commit
c6bfc92df1
@ -309,7 +309,7 @@ public class DefaultHttp2LocalFlowController implements Http2LocalFlowController
|
|||||||
* @throws Http2Exception If too much data is used relative to how much is available.
|
* @throws Http2Exception If too much data is used relative to how much is available.
|
||||||
*/
|
*/
|
||||||
void receiveFlowControlledFrame(int dataLength) throws Http2Exception {
|
void receiveFlowControlledFrame(int dataLength) throws Http2Exception {
|
||||||
assert dataLength > 0;
|
assert dataLength >= 0;
|
||||||
|
|
||||||
// Apply the delta. Even if we throw an exception we want to have taken this delta into account.
|
// Apply the delta. Even if we throw an exception we want to have taken this delta into account.
|
||||||
window -= dataLength;
|
window -= dataLength;
|
||||||
|
@ -53,7 +53,7 @@ public interface Http2LocalFlowController extends Http2FlowController {
|
|||||||
* @param stream the stream for which window space should be freed. The connection stream object
|
* @param stream the stream for which window space should be freed. The connection stream object
|
||||||
* must not be used.
|
* must not be used.
|
||||||
* @param numBytes the number of bytes to be returned to the flow control window.
|
* @param numBytes the number of bytes to be returned to the flow control window.
|
||||||
* @throws Http2Exception if the number of bytes returned exceeds the {@link #unconsumedBytes()}
|
* @throws Http2Exception if the number of bytes returned exceeds the {@link #unconsumedBytes}
|
||||||
* for the stream.
|
* for the stream.
|
||||||
*/
|
*/
|
||||||
void consumeBytes(ChannelHandlerContext ctx, Http2Stream stream, int numBytes) throws Http2Exception;
|
void consumeBytes(ChannelHandlerContext ctx, Http2Stream stream, int numBytes) throws Http2Exception;
|
||||||
|
@ -174,17 +174,37 @@ public class DefaultHttp2ConnectionDecoderTest {
|
|||||||
int processedBytes = data.readableBytes() + padding;
|
int processedBytes = data.readableBytes() + padding;
|
||||||
mockFlowControl(processedBytes);
|
mockFlowControl(processedBytes);
|
||||||
try {
|
try {
|
||||||
decode().onDataRead(ctx, STREAM_ID, data, 10, 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));
|
||||||
verify(localFlow).consumeBytes(eq(ctx), eq(stream), eq(processedBytes));
|
verify(localFlow).consumeBytes(eq(ctx), eq(stream), eq(processedBytes));
|
||||||
|
|
||||||
// Verify that the event was absorbed and not propagated to the oberver.
|
// Verify that the event was absorbed and not propagated to the observer.
|
||||||
verify(listener, never()).onDataRead(eq(ctx), anyInt(), any(ByteBuf.class), anyInt(), anyBoolean());
|
verify(listener, never()).onDataRead(eq(ctx), anyInt(), any(ByteBuf.class), anyInt(), anyBoolean());
|
||||||
} finally {
|
} finally {
|
||||||
data.release();
|
data.release();
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
public void emptyDataFrameShouldApplyFlowControl() throws Exception {
|
||||||
|
final ByteBuf data = EMPTY_BUFFER;
|
||||||
|
int padding = 0;
|
||||||
|
int processedBytes = data.readableBytes() + padding;
|
||||||
|
mockFlowControl(processedBytes);
|
||||||
|
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));
|
||||||
|
|
||||||
|
// 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));
|
||||||
|
} finally {
|
||||||
|
data.release();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@Test(expected = Http2Exception.class)
|
@Test(expected = Http2Exception.class)
|
||||||
public void dataReadForStreamInInvalidStateShouldThrow() throws Exception {
|
public void dataReadForStreamInInvalidStateShouldThrow() throws Exception {
|
||||||
// Throw an exception when checking stream state.
|
// Throw an exception when checking stream state.
|
||||||
|
Loading…
Reference in New Issue
Block a user