Http2FrameCodec WindowUpdate bug

Motivation:
Http2FrameCodec increases the initialWindowSize when the user attempts to increase the connection flow control window. The initialWindowSize should only be touched as a result of a SETTINGS frame, and otherwise may result in flow control getting out of sync with our peer.

Modifications:
- Http2FrameCodec shouldn't update the initialWindowSize when a WindowUpdateFrame is written on the connection channel

Result:
More correct WindowUpdate processing.
This commit is contained in:
Scott Mitchell 2018-01-22 11:17:56 -08:00 committed by GitHub
parent 949c515333
commit 46dac128f7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 26 additions and 6 deletions

View File

@ -305,10 +305,8 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
} }
private void increaseInitialConnectionWindow(int deltaBytes) throws Http2Exception { private void increaseInitialConnectionWindow(int deltaBytes) throws Http2Exception {
Http2LocalFlowController localFlow = connection().local().flowController(); // The LocalFlowController is responsible for detecting over/under flow.
int targetConnectionWindow = localFlow.initialWindowSize() + deltaBytes; connection().local().flowController().incrementWindowSize(connection().connectionStream(), deltaBytes);
localFlow.incrementWindowSize(connection().connectionStream(), deltaBytes);
localFlow.initialWindowSize(targetConnectionWindow);
} }
final boolean consumeBytes(int streamId, int bytes) throws Http2Exception { final boolean consumeBytes(int streamId, int bytes) throws Http2Exception {

View File

@ -478,16 +478,38 @@ public class Http2FrameCodecTest {
} }
@Test @Test
public void streamZeroWindowUpdateIncrementsConnectionWindow() throws Exception { public void streamZeroWindowUpdateIncrementsConnectionWindow() throws Http2Exception {
Http2Connection connection = frameCodec.connection(); Http2Connection connection = frameCodec.connection();
Http2LocalFlowController localFlow = connection.local().flowController(); Http2LocalFlowController localFlow = connection.local().flowController();
int initialWindowSizeBefore = localFlow.initialWindowSize(); int initialWindowSizeBefore = localFlow.initialWindowSize();
Http2Stream connectionStream = connection.connectionStream();
int connectionWindowSizeBefore = localFlow.windowSize(connectionStream);
// We only replenish the flow control window after the amount consumed drops below the following threshold.
// We make the threshold very "high" so that window updates will be sent when the delta is relatively small.
((DefaultHttp2LocalFlowController) localFlow).windowUpdateRatio(connectionStream, .999f);
int windowUpdate = 1024; int windowUpdate = 1024;
channel.write(new DefaultHttp2WindowUpdateFrame(windowUpdate)); channel.write(new DefaultHttp2WindowUpdateFrame(windowUpdate));
assertEquals(initialWindowSizeBefore + windowUpdate, localFlow.initialWindowSize()); // The initial window size is only changed by Http2Settings, so it shouldn't change.
assertEquals(initialWindowSizeBefore, localFlow.initialWindowSize());
// The connection window should be increased by the delta amount.
assertEquals(connectionWindowSizeBefore + windowUpdate, localFlow.windowSize(connectionStream));
}
@Test
public void windowUpdateDoesNotOverflowConnectionWindow() {
Http2Connection connection = frameCodec.connection();
Http2LocalFlowController localFlow = connection.local().flowController();
int initialWindowSizeBefore = localFlow.initialWindowSize();
channel.write(new DefaultHttp2WindowUpdateFrame(Integer.MAX_VALUE));
// The initial window size is only changed by Http2Settings, so it shouldn't change.
assertEquals(initialWindowSizeBefore, localFlow.initialWindowSize());
// The connection window should be increased by the delta amount.
assertEquals(Integer.MAX_VALUE, localFlow.windowSize(connection.connectionStream()));
} }
@Test @Test