HTTP/2 DefaultHttp2RemoteFlowController frame merging with padding bug
Motivation: DefaultHttp2RemoteFlowController does not correctly account for the padding in the event frames are merged. This causes the internal stat of DefaultHttp2RemoteFlowController to become corrupt and can result in attempting to write frames when there are none. Modifications: - Update DefaultHttp2RemoteFlowController to account for frame sizes not necessarily adding together. Result: DefaultHttp2RemoteFlowController internal state does not become corrupt when padding is present. Fixes https://github.com/netty/netty/issues/4573
This commit is contained in:
parent
ca305d86fb
commit
e8850072e2
@ -357,10 +357,11 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder {
|
|||||||
|
|
||||||
@Override
|
@Override
|
||||||
public boolean merge(ChannelHandlerContext ctx, Http2RemoteFlowController.FlowControlled next) {
|
public boolean merge(ChannelHandlerContext ctx, Http2RemoteFlowController.FlowControlled next) {
|
||||||
if (FlowControlledData.class != next.getClass()) {
|
FlowControlledData nextData;
|
||||||
|
if (FlowControlledData.class != next.getClass() ||
|
||||||
|
Integer.MAX_VALUE - (nextData = (FlowControlledData) next).size() < size()) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
FlowControlledData nextData = (FlowControlledData) next;
|
|
||||||
nextData.queue.copyTo(queue);
|
nextData.queue.copyTo(queue);
|
||||||
// Given that we're merging data into a frame it doesn't really make sense to accumulate padding.
|
// Given that we're merging data into a frame it doesn't really make sense to accumulate padding.
|
||||||
padding = Math.max(padding, nextData.padding);
|
padding = Math.max(padding, nextData.padding);
|
||||||
|
@ -404,9 +404,21 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll
|
|||||||
@Override
|
@Override
|
||||||
void enqueueFrame(FlowControlled frame) {
|
void enqueueFrame(FlowControlled frame) {
|
||||||
FlowControlled last = pendingWriteQueue.peekLast();
|
FlowControlled last = pendingWriteQueue.peekLast();
|
||||||
if (last == null || !last.merge(ctx, frame)) {
|
if (last == null) {
|
||||||
pendingWriteQueue.offer(frame);
|
enqueueFrameWithoutMerge(frame);
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
int lastSize = last.size();
|
||||||
|
if (last.merge(ctx, frame)) {
|
||||||
|
incrementPendingBytes(last.size() - lastSize, true);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
enqueueFrameWithoutMerge(frame);
|
||||||
|
}
|
||||||
|
|
||||||
|
private void enqueueFrameWithoutMerge(FlowControlled frame) {
|
||||||
|
pendingWriteQueue.offer(frame);
|
||||||
// This must be called after adding to the queue in order so that hasFrame() is
|
// This must be called after adding to the queue in order so that hasFrame() is
|
||||||
// updated before updating the stream state.
|
// updated before updating the stream state.
|
||||||
incrementPendingBytes(frame.size(), true);
|
incrementPendingBytes(frame.size(), true);
|
||||||
|
@ -212,6 +212,23 @@ public abstract class DefaultHttp2RemoteFlowControllerTest {
|
|||||||
assertFalse(controller.isWritable(stream(STREAM_A)));
|
assertFalse(controller.isWritable(stream(STREAM_A)));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
public void flowControllerCorrectlyAccountsForBytesWithMerge() throws Http2Exception {
|
||||||
|
controller.initialWindowSize(112); // This must be more than the total merged frame size 110
|
||||||
|
FakeFlowControlled data1 = new FakeFlowControlled(5, 2, true);
|
||||||
|
FakeFlowControlled data2 = new FakeFlowControlled(5, 100, true);
|
||||||
|
sendData(STREAM_A, data1);
|
||||||
|
sendData(STREAM_A, data2);
|
||||||
|
data1.assertNotWritten();
|
||||||
|
data1.assertNotWritten();
|
||||||
|
data2.assertMerged();
|
||||||
|
controller.writePendingBytes();
|
||||||
|
data1.assertFullyWritten();
|
||||||
|
data2.assertNotWritten();
|
||||||
|
verify(listener, never()).writabilityChanged(stream(STREAM_A));
|
||||||
|
assertTrue(controller.isWritable(stream(STREAM_A)));
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void stalledStreamShouldQueuePayloads() throws Http2Exception {
|
public void stalledStreamShouldQueuePayloads() throws Http2Exception {
|
||||||
controller.initialWindowSize(0);
|
controller.initialWindowSize(0);
|
||||||
@ -953,9 +970,10 @@ public abstract class DefaultHttp2RemoteFlowControllerTest {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private static final class FakeFlowControlled implements Http2RemoteFlowController.FlowControlled {
|
private static final class FakeFlowControlled implements Http2RemoteFlowController.FlowControlled {
|
||||||
|
private int currentPadding;
|
||||||
private int currentSize;
|
private int currentPayloadSize;
|
||||||
private int originalSize;
|
private int originalPayloadSize;
|
||||||
|
private int originalPadding;
|
||||||
private boolean writeCalled;
|
private boolean writeCalled;
|
||||||
private final boolean mergeable;
|
private final boolean mergeable;
|
||||||
private boolean merged;
|
private boolean merged;
|
||||||
@ -963,20 +981,26 @@ public abstract class DefaultHttp2RemoteFlowControllerTest {
|
|||||||
private Throwable t;
|
private Throwable t;
|
||||||
|
|
||||||
private FakeFlowControlled(int size) {
|
private FakeFlowControlled(int size) {
|
||||||
this.currentSize = size;
|
this(size, false);
|
||||||
this.originalSize = size;
|
|
||||||
this.mergeable = false;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private FakeFlowControlled(int size, boolean mergeable) {
|
private FakeFlowControlled(int size, boolean mergeable) {
|
||||||
this.currentSize = size;
|
this(size, 0, mergeable);
|
||||||
this.originalSize = size;
|
}
|
||||||
|
|
||||||
|
private FakeFlowControlled(int payloadSize, int padding, boolean mergeable) {
|
||||||
|
currentPayloadSize = originalPayloadSize = payloadSize;
|
||||||
|
currentPadding = originalPadding = padding;
|
||||||
this.mergeable = mergeable;
|
this.mergeable = mergeable;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public int size() {
|
public int size() {
|
||||||
return currentSize;
|
return currentPayloadSize + currentPadding;
|
||||||
|
}
|
||||||
|
|
||||||
|
private int originalSize() {
|
||||||
|
return originalPayloadSize + originalPadding;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
@ -990,28 +1014,36 @@ public abstract class DefaultHttp2RemoteFlowControllerTest {
|
|||||||
|
|
||||||
@Override
|
@Override
|
||||||
public void write(ChannelHandlerContext ctx, int allowedBytes) {
|
public void write(ChannelHandlerContext ctx, int allowedBytes) {
|
||||||
if (allowedBytes <= 0 && currentSize != 0) {
|
if (allowedBytes <= 0 && size() != 0) {
|
||||||
// Write has been called but no data can be written
|
// Write has been called but no data can be written
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
writeCalled = true;
|
writeCalled = true;
|
||||||
int written = Math.min(currentSize, allowedBytes);
|
int written = Math.min(size(), allowedBytes);
|
||||||
currentSize -= written;
|
if (written > currentPayloadSize) {
|
||||||
|
written -= currentPayloadSize;
|
||||||
|
currentPayloadSize = 0;
|
||||||
|
currentPadding -= written;
|
||||||
|
} else {
|
||||||
|
currentPayloadSize -= written;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public boolean merge(ChannelHandlerContext ctx, Http2RemoteFlowController.FlowControlled next) {
|
public boolean merge(ChannelHandlerContext ctx, Http2RemoteFlowController.FlowControlled next) {
|
||||||
if (mergeable && next instanceof FakeFlowControlled) {
|
if (mergeable && next instanceof FakeFlowControlled) {
|
||||||
this.originalSize += ((FakeFlowControlled) next).originalSize;
|
FakeFlowControlled ffcNext = (FakeFlowControlled) next;
|
||||||
this.currentSize += ((FakeFlowControlled) next).originalSize;
|
originalPayloadSize += ffcNext.originalPayloadSize;
|
||||||
((FakeFlowControlled) next).merged = true;
|
currentPayloadSize += ffcNext.originalPayloadSize;
|
||||||
|
currentPadding = originalPadding = Math.max(originalPadding, ffcNext.originalPadding);
|
||||||
|
ffcNext.merged = true;
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
public int written() {
|
public int written() {
|
||||||
return originalSize - currentSize;
|
return originalSize() - size();
|
||||||
}
|
}
|
||||||
|
|
||||||
public void assertNotWritten() {
|
public void assertNotWritten() {
|
||||||
@ -1029,7 +1061,8 @@ public abstract class DefaultHttp2RemoteFlowControllerTest {
|
|||||||
|
|
||||||
public void assertFullyWritten() {
|
public void assertFullyWritten() {
|
||||||
assertTrue(writeCalled);
|
assertTrue(writeCalled);
|
||||||
assertEquals(0, currentSize);
|
assertEquals(0, currentPayloadSize);
|
||||||
|
assertEquals(0, currentPadding);
|
||||||
}
|
}
|
||||||
|
|
||||||
public boolean assertMerged() {
|
public boolean assertMerged() {
|
||||||
|
Loading…
Reference in New Issue
Block a user