Changes oghttp2 flow control behavior when sending a large WINDOW_UPDATE. When sending a WINDOW_UPDATE frame that raises the advertised window size above the current limit, this change causes oghttp2 to update the limit to the advertised window size. This aligns oghttp2 behavior more closely with nghttp2, for ease of transition. PiperOrigin-RevId: 481757774
diff --git a/quiche/http2/adapter/adapter_impl_comparison_test.cc b/quiche/http2/adapter/adapter_impl_comparison_test.cc index 364e996..aaa2f99 100644 --- a/quiche/http2/adapter/adapter_impl_comparison_test.cc +++ b/quiche/http2/adapter/adapter_impl_comparison_test.cc
@@ -111,6 +111,8 @@ nghttp2_window = nghttp2_adapter->GetReceiveWindowSize(); oghttp2_adapter->ProcessBytes(frames); + // Marking the data consumed causes a window update, which is reflected in the + // advertised window size. oghttp2_adapter->MarkDataConsumedForStream(oghttp2_stream_id, kNumFrames * kMaxFrameSize); result = oghttp2_adapter->Send(); @@ -120,8 +122,7 @@ const int kMinExpectation = (kInitialFlowControlWindow + kConnectionWindowIncrease) / 2; EXPECT_GT(nghttp2_window, kMinExpectation); - // BUG! oghttp2 does not maintain the larger window persistently. - EXPECT_LT(oghttp2_window, kMinExpectation); + EXPECT_GT(oghttp2_window, kMinExpectation); } TEST(AdapterImplComparisonTest, ServerHandlesFrames) {
diff --git a/quiche/http2/adapter/nghttp2_adapter_test.cc b/quiche/http2/adapter/nghttp2_adapter_test.cc index 9bf5bd2..aff31e4 100644 --- a/quiche/http2/adapter/nghttp2_adapter_test.cc +++ b/quiche/http2/adapter/nghttp2_adapter_test.cc
@@ -3265,6 +3265,80 @@ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS})); } +TEST(NgHttp2AdapterTest, WindowUpdateRaisesFlowControlWindowLimit) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + + const std::string data_chunk(kDefaultFramePayloadSizeLimit, 'a'); + const std::string request = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/"}}, + /*fin=*/false) + .Serialize(); + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + + adapter->ProcessBytes(request); + + // Updates the advertised window for the connection and stream 1. + adapter->SubmitWindowUpdate(0, 2 * kDefaultFramePayloadSizeLimit); + adapter->SubmitWindowUpdate(1, 2 * kDefaultFramePayloadSizeLimit); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, 4, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, 4, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 1, 4, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 1, 4, 0x0, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + + // Verifies the advertised window. + EXPECT_EQ(kInitialFlowControlWindowSize + 2 * kDefaultFramePayloadSizeLimit, + adapter->GetReceiveWindowSize()); + EXPECT_EQ(kInitialFlowControlWindowSize + 2 * kDefaultFramePayloadSizeLimit, + adapter->GetStreamReceiveWindowSize(1)); + + const std::string request_body = TestFrameSequence() + .Data(1, data_chunk) + .Data(1, data_chunk) + .Data(1, data_chunk) + .Data(1, data_chunk) + .Data(1, data_chunk) + .Serialize(); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)).Times(5); + EXPECT_CALL(visitor, OnBeginDataForStream(1, _)).Times(5); + EXPECT_CALL(visitor, OnDataForStream(1, _)).Times(5); + + // DATA frames on stream 1 consume most of the window. + adapter->ProcessBytes(request_body); + EXPECT_EQ(kInitialFlowControlWindowSize - 3 * kDefaultFramePayloadSizeLimit, + adapter->GetReceiveWindowSize()); + EXPECT_EQ(kInitialFlowControlWindowSize - 3 * kDefaultFramePayloadSizeLimit, + adapter->GetStreamReceiveWindowSize(1)); + + // Marking the data consumed should result in an advertised window larger than + // the initial window. + adapter->MarkDataConsumedForStream(1, 4 * kDefaultFramePayloadSizeLimit); + EXPECT_GT(adapter->GetReceiveWindowSize(), kInitialFlowControlWindowSize); + EXPECT_GT(adapter->GetStreamReceiveWindowSize(1), + kInitialFlowControlWindowSize); +} + TEST(NgHttp2AdapterTest, ConnectionErrorOnControlFrameSent) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc index 13c330b..a98dcd1 100644 --- a/quiche/http2/adapter/oghttp2_adapter_test.cc +++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -3832,6 +3832,84 @@ EXPECT_EQ(peer_window, kInitialFlowControlWindowSize); } +TEST(OgHttp2AdapterTest, WindowUpdateRaisesFlowControlWindowLimit) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options; + options.perspective = Perspective::kServer; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + const std::string data_chunk(kDefaultFramePayloadSizeLimit, 'a'); + const std::string request = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/"}}, + /*fin=*/false) + .Serialize(); + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + + adapter->ProcessBytes(request); + + // Updates the advertised window for the connection and stream 1. + adapter->SubmitWindowUpdate(0, 2 * kDefaultFramePayloadSizeLimit); + adapter->SubmitWindowUpdate(1, 2 * kDefaultFramePayloadSizeLimit); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, 4, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, 4, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 1, 4, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 1, 4, 0x0, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + + // Verifies the advertised window. + EXPECT_EQ(kInitialFlowControlWindowSize + 2 * kDefaultFramePayloadSizeLimit, + adapter->GetReceiveWindowSize()); + EXPECT_EQ(kInitialFlowControlWindowSize + 2 * kDefaultFramePayloadSizeLimit, + adapter->GetStreamReceiveWindowSize(1)); + + const std::string request_body = TestFrameSequence() + .Data(1, data_chunk) + .Data(1, data_chunk) + .Data(1, data_chunk) + .Data(1, data_chunk) + .Data(1, data_chunk) + .Serialize(); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)).Times(5); + EXPECT_CALL(visitor, OnBeginDataForStream(1, _)).Times(5); + EXPECT_CALL(visitor, OnDataForStream(1, _)).Times(5); + + // DATA frames on stream 1 consume most of the window. + adapter->ProcessBytes(request_body); + EXPECT_EQ(kInitialFlowControlWindowSize - 3 * kDefaultFramePayloadSizeLimit, + adapter->GetReceiveWindowSize()); + EXPECT_EQ(kInitialFlowControlWindowSize - 3 * kDefaultFramePayloadSizeLimit, + adapter->GetStreamReceiveWindowSize(1)); + + // Marking the data consumed should result in an advertised window larger than + // the initial window. + adapter->MarkDataConsumedForStream(1, 4 * kDefaultFramePayloadSizeLimit); + EXPECT_GT(adapter->GetReceiveWindowSize(), kInitialFlowControlWindowSize); + EXPECT_GT(adapter->GetStreamReceiveWindowSize(1), + kInitialFlowControlWindowSize); +} + TEST(OgHttp2AdapterTest, MarkDataConsumedForNonexistentStream) { DataSavingVisitor visitor; OgHttp2Adapter::Options options;
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index 44ec739..96b3ab1 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -1931,10 +1931,24 @@ int32_t delta) { if (stream_id == 0) { connection_window_manager_.IncreaseWindow(delta); + // TODO(b/181586191): Provide an explicit way to set the desired window + // limit, remove the upsize-on-window-update behavior. + const int64_t current_window = + connection_window_manager_.CurrentWindowSize(); + if (current_window > connection_window_manager_.WindowSizeLimit()) { + connection_window_manager_.SetWindowSizeLimit(current_window); + } } else { auto iter = stream_map_.find(stream_id); if (iter != stream_map_.end()) { - iter->second.window_manager.IncreaseWindow(delta); + WindowManager& manager = iter->second.window_manager; + manager.IncreaseWindow(delta); + // TODO(b/181586191): Provide an explicit way to set the desired window + // limit, remove the upsize-on-window-update behavior. + const int64_t current_window = manager.CurrentWindowSize(); + if (current_window > manager.WindowSizeLimit()) { + manager.SetWindowSizeLimit(current_window); + } } } }