Handle outbound SETTINGS_MAX_FRAME_SIZE in oghttp2. PiperOrigin-RevId: 434784601
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 0294344..fe4e32c 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -2756,6 +2756,143 @@ EXPECT_EQ(result, NGHTTP2_ERR_CALLBACK_FAILURE); } +TEST(NgHttp2AdapterTest, MaxFrameSizeSettingNotAppliedBeforeAck) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); + + const uint32_t large_frame_size = kDefaultFramePayloadSizeLimit + 42; + adapter->SubmitSettings({{MAX_FRAME_SIZE, large_frame_size}}); + const int32_t stream_id = + adapter->SubmitRequest(ToHeaders({{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + /*data_source=*/nullptr, /*user_data=*/nullptr); + EXPECT_GT(stream_id, 0); + EXPECT_TRUE(adapter->want_write()); + + testing::InSequence s; + + // Client preface (SETTINGS with MAX_FRAME_SIZE) and request HEADERS + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + absl::string_view data = visitor.data(); + EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(data, + EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string server_frames = + TestFrameSequence() + .ServerPreface() + .Headers(1, {{":status", "200"}}, /*fin=*/false) + .Data(1, std::string(large_frame_size, 'a')) + .Serialize(); + + // Server preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + // Response HEADERS. Because the SETTINGS with MAX_FRAME_SIZE was not + // acknowledged, the large DATA is treated as a connection error. Note that + // nghttp2 does not deliver any DATA or connection error events. + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "200")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + + const int64_t process_result = adapter->ProcessBytes(server_frames); + EXPECT_EQ(server_frames.size(), static_cast<size_t>(process_result)); + + EXPECT_TRUE(adapter->want_write()); + + EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(GOAWAY, 0, _, 0x0, + static_cast<int>(Http2ErrorCode::FRAME_SIZE_ERROR))); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::GOAWAY})); +} + +TEST(NgHttp2AdapterTest, MaxFrameSizeSettingAppliedAfterAck) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); + + const uint32_t large_frame_size = kDefaultFramePayloadSizeLimit + 42; + adapter->SubmitSettings({{MAX_FRAME_SIZE, large_frame_size}}); + const int32_t stream_id = + adapter->SubmitRequest(ToHeaders({{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + /*data_source=*/nullptr, /*user_data=*/nullptr); + EXPECT_GT(stream_id, 0); + EXPECT_TRUE(adapter->want_write()); + + testing::InSequence s; + + // Client preface (SETTINGS with MAX_FRAME_SIZE) and request HEADERS + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + absl::string_view data = visitor.data(); + EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(data, + EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string server_frames = + TestFrameSequence() + .ServerPreface() + .SettingsAck() + .Headers(1, {{":status", "200"}}, /*fin=*/false) + .Data(1, std::string(large_frame_size, 'a')) + .Serialize(); + + // Server preface (empty SETTINGS) and ack of SETTINGS. + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0x1)); + EXPECT_CALL(visitor, OnSettingsAck()); + + // Response HEADERS and DATA. Because the SETTINGS with MAX_FRAME_SIZE was + // acknowledged, the large DATA is accepted without any error. + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "200")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, large_frame_size, DATA, 0x0)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, large_frame_size)); + EXPECT_CALL(visitor, OnDataForStream(1, _)); + + const int64_t process_result = adapter->ProcessBytes(server_frames); + EXPECT_EQ(server_frames.size(), static_cast<size_t>(process_result)); + + // Client ack of SETTINGS. + EXPECT_TRUE(adapter->want_write()); + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS})); +} + TEST(NgHttp2AdapterTest, ConnectionErrorOnControlFrameSent) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index b7fa20c..1925173 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -2472,6 +2472,147 @@ EXPECT_LT(result, 0); } +TEST(OgHttp2AdapterTest, MaxFrameSizeSettingNotAppliedBeforeAck) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + const uint32_t large_frame_size = kDefaultFramePayloadSizeLimit + 42; + adapter->SubmitSettings({{MAX_FRAME_SIZE, large_frame_size}}); + const int32_t stream_id = + adapter->SubmitRequest(ToHeaders({{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + /*data_source=*/nullptr, /*user_data=*/nullptr); + EXPECT_GT(stream_id, 0); + EXPECT_TRUE(adapter->want_write()); + + testing::InSequence s; + + // Client preface (SETTINGS with MAX_FRAME_SIZE) and request HEADERS + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + absl::string_view data = visitor.data(); + EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(data, + EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string server_frames = + TestFrameSequence() + .ServerPreface() + .Headers(1, {{":status", "200"}}, /*fin=*/false) + .Data(1, std::string(large_frame_size, 'a')) + .Serialize(); + + // Server preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + // Response HEADERS. Because the SETTINGS with MAX_FRAME_SIZE was not + // acknowledged, the large DATA is treated as a connection error. Note that + // oghttp2 delivers the DATA frame header and connection error events. + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "200")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, large_frame_size, DATA, 0x0)); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); + + const int64_t process_result = adapter->ProcessBytes(server_frames); + EXPECT_EQ(server_frames.size(), static_cast<size_t>(process_result)); + + EXPECT_TRUE(adapter->want_write()); + + EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(GOAWAY, 0, _, 0x0, + static_cast<int>(Http2ErrorCode::FRAME_SIZE_ERROR))); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::GOAWAY})); +} + +TEST(OgHttp2AdapterTest, MaxFrameSizeSettingAppliedAfterAck) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + const uint32_t large_frame_size = kDefaultFramePayloadSizeLimit + 42; + adapter->SubmitSettings({{MAX_FRAME_SIZE, large_frame_size}}); + const int32_t stream_id = + adapter->SubmitRequest(ToHeaders({{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + /*data_source=*/nullptr, /*user_data=*/nullptr); + EXPECT_GT(stream_id, 0); + EXPECT_TRUE(adapter->want_write()); + + testing::InSequence s; + + // Client preface (SETTINGS with MAX_FRAME_SIZE) and request HEADERS + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + absl::string_view data = visitor.data(); + EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(data, + EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string server_frames = + TestFrameSequence() + .ServerPreface() + .SettingsAck() + .Headers(1, {{":status", "200"}}, /*fin=*/false) + .Data(1, std::string(large_frame_size, 'a')) + .Serialize(); + + // Server preface (empty SETTINGS) and ack of SETTINGS. + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0x1)); + EXPECT_CALL(visitor, OnSettingsAck()); + + // Response HEADERS and DATA. Because the SETTINGS with MAX_FRAME_SIZE was + // acknowledged, the large DATA is accepted without any error. + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "200")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, large_frame_size, DATA, 0x0)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, large_frame_size)); + EXPECT_CALL(visitor, OnDataForStream(1, _)); + + const int64_t process_result = adapter->ProcessBytes(server_frames); + EXPECT_EQ(server_frames.size(), static_cast<size_t>(process_result)); + + // Client ack of SETTINGS. + EXPECT_TRUE(adapter->want_write()); + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS})); +} + TEST(OgHttp2AdapterTest, ClientForbidsPushPromise) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; @@ -4263,8 +4404,6 @@ // Now let the client ack the MAX_FRAME_SIZE SETTINGS and send a DATA frame to // overflow the connection-level window. The result should be a GOAWAY. - // TODO(b/223471995): The result is a GOAWAY, but not for the right reason. - // Fix oghttp2 to handle outbound SETTINGS with MAX_FRAME_SIZE. const std::string overflow_frames = TestFrameSequence() .SettingsAck() @@ -4274,16 +4413,17 @@ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0x1)); EXPECT_CALL(visitor, OnSettingsAck()); EXPECT_CALL(visitor, OnFrameHeader(1, window_overflow_bytes, DATA, 0x0)); - EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kFlowControlError)); // No further frame data is delivered. process_result = adapter->ProcessBytes(overflow_frames); EXPECT_EQ(overflow_frames.size(), static_cast<size_t>(process_result)); EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0)); - EXPECT_CALL(visitor, - OnFrameSent(GOAWAY, 0, _, 0x0, - static_cast<int>(Http2ErrorCode::FRAME_SIZE_ERROR))); + EXPECT_CALL( + visitor, + OnFrameSent(GOAWAY, 0, _, 0x0, + static_cast<int>(Http2ErrorCode::FLOW_CONTROL_ERROR))); send_result = adapter->Send(); EXPECT_EQ(0, send_result); @@ -4344,8 +4484,6 @@ // Now let the client ack the MAX_FRAME_SIZE SETTINGS and send a DATA frame to // overflow the connection-level window. The result should be a GOAWAY. - // TODO(b/223471995): The result is a GOAWAY, but not for the right reason. - // Fix oghttp2 to handle outbound SETTINGS with MAX_FRAME_SIZE. const std::string overflow_frames = TestFrameSequence() .SettingsAck() @@ -4355,7 +4493,7 @@ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0x1)); EXPECT_CALL(visitor, OnSettingsAck()); EXPECT_CALL(visitor, OnFrameHeader(1, window_overflow_bytes, DATA, 0x0)); - EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kFlowControlError)); const size_t chunk_length = 16384; ASSERT_GE(overflow_frames.size(), chunk_length); @@ -4364,9 +4502,10 @@ EXPECT_EQ(chunk_length, static_cast<size_t>(process_result)); EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0)); - EXPECT_CALL(visitor, - OnFrameSent(GOAWAY, 0, _, 0x0, - static_cast<int>(Http2ErrorCode::FRAME_SIZE_ERROR))); + EXPECT_CALL( + visitor, + OnFrameSent(GOAWAY, 0, _, 0x0, + static_cast<int>(Http2ErrorCode::FLOW_CONTROL_ERROR))); send_result = adapter->Send(); EXPECT_EQ(0, send_result);
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 9a120c3..25cd0e9 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -1591,8 +1591,10 @@ UpdateStreamReceiveWindowSizes(value); initial_stream_receive_window_ = value; break; - case ENABLE_PUSH: case MAX_FRAME_SIZE: + decoder_.SetMaxFrameSize(value); + break; + case ENABLE_PUSH: case MAX_HEADER_LIST_SIZE: case ENABLE_CONNECT_PROTOCOL: QUICHE_VLOG(2)