From: Tanvi Jagtap Date: Tue, 2 Dec 2025 14:49:23 +0000 (-0800) Subject: [PH2][Settings][Refactor] Step 3.3 X-Git-Url: https://git.feebdaed.xyz/?a=commitdiff_plain;h=af0f055a825519c00df1bc39e9125c98791311b3;p=0xmirror%2Fgrpc.git [PH2][Settings][Refactor] Step 3.3 1. Removes unused includes of http2_settings_manager.h 2. Moves settings ACK handling into SettingsPromiseManager from Http2SettingsManager 3. Deletes `MaybeSendAck` related tests from http2_settings_test.cc 4. Moved tests as-is into settings_timeout_manager_test.cc from http2_transport_test.cc PiperOrigin-RevId: 839247137 --- diff --git a/src/core/BUILD b/src/core/BUILD index fc51295156..909cd27999 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -8301,6 +8301,7 @@ grpc_cc_library( "try_seq", "useful", ":chttp2_flow_control", + ":slice_buffer", "//:chttp2_frame", "//:gpr_platform", "//:promise", diff --git a/src/core/ext/transport/chttp2/transport/http2_client_transport.cc b/src/core/ext/transport/chttp2/transport/http2_client_transport.cc index 86a115a03e..d08d6401cc 100644 --- a/src/core/ext/transport/chttp2/transport/http2_client_transport.cc +++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.cc @@ -44,7 +44,6 @@ #include "src/core/ext/transport/chttp2/transport/frame.h" #include "src/core/ext/transport/chttp2/transport/header_assembler.h" #include "src/core/ext/transport/chttp2/transport/http2_settings.h" -#include "src/core/ext/transport/chttp2/transport/http2_settings_manager.h" #include "src/core/ext/transport/chttp2/transport/http2_status.h" #include "src/core/ext/transport/chttp2/transport/http2_transport.h" #include "src/core/ext/transport/chttp2/transport/http2_ztrace_collector.h" @@ -1296,9 +1295,6 @@ void Http2ClientTransport::EnforceLatestIncomingSettings() { } auto Http2ClientTransport::WaitForSettingsTimeoutOnDone() { - // TODO(tjagtap) : [PH2][P1][Settings] : Handle Transport Close case. - // TODO(tjagtap) : [PH2][P1][Settings] Move this out of the transport into a - // Settings class. return [self = RefAsSubclass()](absl::Status status) { if (!status.ok()) { GRPC_UNUSED absl::Status result = self->HandleError( diff --git a/src/core/ext/transport/chttp2/transport/http2_client_transport.h b/src/core/ext/transport/chttp2/transport/http2_client_transport.h index 9f503e1d1e..e34fb45eb9 100644 --- a/src/core/ext/transport/chttp2/transport/http2_client_transport.h +++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.h @@ -39,7 +39,6 @@ #include "src/core/ext/transport/chttp2/transport/goaway.h" #include "src/core/ext/transport/chttp2/transport/hpack_encoder.h" #include "src/core/ext/transport/chttp2/transport/hpack_parser.h" -#include "src/core/ext/transport/chttp2/transport/http2_settings_manager.h" #include "src/core/ext/transport/chttp2/transport/http2_settings_promises.h" #include "src/core/ext/transport/chttp2/transport/http2_status.h" #include "src/core/ext/transport/chttp2/transport/http2_transport.h" diff --git a/src/core/ext/transport/chttp2/transport/http2_settings_manager.cc b/src/core/ext/transport/chttp2/transport/http2_settings_manager.cc index 4d0b6deceb..00a3cba455 100644 --- a/src/core/ext/transport/chttp2/transport/http2_settings_manager.cc +++ b/src/core/ext/transport/chttp2/transport/http2_settings_manager.cc @@ -52,10 +52,6 @@ std::optional Http2SettingsManager::MaybeSendUpdate() { return frame; } -uint32_t Http2SettingsManager::MaybeSendAck() { - return std::exchange(num_acks_to_send_, 0); -} - bool Http2SettingsManager::AckLastSend() { if (update_state_ != UpdateState::kSending) return false; update_state_ = UpdateState::kIdle; diff --git a/src/core/ext/transport/chttp2/transport/http2_settings_manager.h b/src/core/ext/transport/chttp2/transport/http2_settings_manager.h index 7d026611c2..6acebf76d9 100644 --- a/src/core/ext/transport/chttp2/transport/http2_settings_manager.h +++ b/src/core/ext/transport/chttp2/transport/http2_settings_manager.h @@ -60,14 +60,6 @@ class Http2SettingsManager { // This function is not idempotent. std::optional MaybeSendUpdate(); - // Returns 0 if we don't need to send a SETTINGS ACK frame to the peer. - // Returns n>0 if we need to send n SETTINGS ACK frames to the peer. - // Transport MUST send one SETTINGS ACK frame for each count returned by this - // function to the peer. - // This function is not idempotent. - uint32_t MaybeSendAck(); - void OnSettingsReceived() { ++num_acks_to_send_; } - // To be called from a promise based HTTP2 transport only http2::Http2ErrorCode ApplyIncomingSettings( const std::vector& settings) { @@ -144,9 +136,6 @@ class Http2SettingsManager { Http2Settings local_; Http2Settings sent_; Http2Settings acked_; - - // Number of incoming SETTINGS frames that we have received but not ACKed yet. - uint32_t num_acks_to_send_ = 0; }; } // namespace grpc_core diff --git a/src/core/ext/transport/chttp2/transport/http2_settings_promises.h b/src/core/ext/transport/chttp2/transport/http2_settings_promises.h index a54791da6d..2fd4c666b1 100644 --- a/src/core/ext/transport/chttp2/transport/http2_settings_promises.h +++ b/src/core/ext/transport/chttp2/transport/http2_settings_promises.h @@ -42,6 +42,7 @@ #include "src/core/lib/promise/race.h" #include "src/core/lib/promise/sleep.h" #include "src/core/lib/promise/try_seq.h" +#include "src/core/lib/slice/slice_buffer.h" #include "src/core/util/grpc_check.h" #include "src/core/util/ref_counted.h" #include "src/core/util/time.h" @@ -64,7 +65,6 @@ namespace grpc_core { // receive and process the SETTINGS ACK. class SettingsPromiseManager : public RefCounted { // TODO(tjagtap) [PH2][P1][Settings] : Add new DCHECKs - // TODO(tjagtap) [PH2][P1][Settings] : Refactor full class public: SettingsPromiseManager() = default; // Not copyable, movable or assignable. @@ -164,7 +164,7 @@ class SettingsPromiseManager : public RefCounted { // Buffered to apply settings at start of next write cycle, only after // SETTINGS ACK is written to the endpoint. void BufferPeerSettings(std::vector&& settings) { - settings_.OnSettingsReceived(); + ++num_acks_to_send_; pending_peer_settings_.reserve(pending_peer_settings_.size() + settings.size()); pending_peer_settings_.insert(pending_peer_settings_.end(), @@ -200,15 +200,15 @@ class SettingsPromiseManager : public RefCounted { WillSendSettings(); } } - const uint32_t num_acks = settings_.MaybeSendAck(); - if (num_acks > 0) { - std::vector ack_frames(num_acks); - for (uint32_t i = 0; i < num_acks; ++i) { + if (num_acks_to_send_ > 0) { + GRPC_SETTINGS_TIMEOUT_DLOG << "Sending " << num_acks_to_send_ + << " settings ACK frames"; + std::vector ack_frames(num_acks_to_send_); + for (uint32_t i = 0; i < num_acks_to_send_; ++i) { ack_frames[i] = Http2SettingsFrame{true, {}}; } Serialize(absl::MakeSpan(ack_frames), output_buf); - GRPC_SETTINGS_TIMEOUT_DLOG << "Sending " << num_acks - << " settings ACK frames"; + num_acks_to_send_ = 0; } } @@ -308,6 +308,8 @@ class SettingsPromiseManager : public RefCounted { ////////////////////////////////////////////////////////////////////////////// // Data Members for SETTINGS being received from the peer. std::vector pending_peer_settings_; + // Number of incoming SETTINGS frames that we have received but not ACKed yet. + uint32_t num_acks_to_send_ = 0; }; } // namespace grpc_core diff --git a/test/core/transport/chttp2/BUILD b/test/core/transport/chttp2/BUILD index cf208af6c2..4b90b14f9f 100644 --- a/test/core/transport/chttp2/BUILD +++ b/test/core/transport/chttp2/BUILD @@ -937,6 +937,7 @@ grpc_cc_test( "absl/log:log", "absl/status", "absl/strings", + "absl/types:span", "gtest", ], uses_polling = False, @@ -949,10 +950,12 @@ grpc_cc_test( "//:gpr", "//:grpc", "//:orphanable", + "//:ref_counted_ptr", "//src/core:1999", "//src/core:arena", "//src/core:call_spine", "//src/core:channel_args", + "//src/core:chttp2_flow_control", "//src/core:default_event_engine", "//src/core:http2_client_transport", "//src/core:http2_settings", @@ -966,6 +969,8 @@ grpc_cc_test( "//src/core:notification", "//src/core:poll", "//src/core:sleep", + "//src/core:slice", + "//src/core:slice_buffer", "//src/core:time", "//src/core:transport_common", "//src/core:try_join", diff --git a/test/core/transport/chttp2/http2_settings_test.cc b/test/core/transport/chttp2/http2_settings_test.cc index dcf4be8328..d72c2b5490 100644 --- a/test/core/transport/chttp2/http2_settings_test.cc +++ b/test/core/transport/chttp2/http2_settings_test.cc @@ -644,41 +644,6 @@ TEST(Http2SettingsManagerTest, Http2ErrorCode::kConnectError); } -TEST(Http2SettingsManagerTest, NoAckNeededInitially) { - // No ACK should be sent initially. - Http2SettingsManager settings_manager; - EXPECT_EQ(settings_manager.MaybeSendAck(), 0u); -} - -TEST(Http2SettingsManagerTest, AckNeededAfterEmptySettings) { - // If we receive an empty SETTINGS frame, we should send an ACK. - Http2SettingsManager settings_manager; - settings_manager.OnSettingsReceived(); - EXPECT_EQ(settings_manager.MaybeSendAck(), 1u); - EXPECT_EQ(settings_manager.MaybeSendAck(), 0u); -} - -TEST(Http2SettingsManagerTest, AckNeededAfterValidSettings) { - // If we receive a valid SETTINGS frame, we should send an ACK. - Http2SettingsManager settings_manager; - std::vector settings = { - {Http2Settings::kHeaderTableSizeWireId, 1000}, - {Http2Settings::kMaxConcurrentStreamsWireId, 200}}; - settings_manager.OnSettingsReceived(); - EXPECT_EQ(settings_manager.MaybeSendAck(), 1u); - EXPECT_EQ(settings_manager.MaybeSendAck(), 0u); -} - -TEST(Http2SettingsManagerTest, MultipleAcksNeeded) { - // If we receive multiple SETTINGS frames before sending an ACK, - // we should send an ACK for each. - Http2SettingsManager settings_manager; - settings_manager.OnSettingsReceived(); - settings_manager.OnSettingsReceived(); - EXPECT_EQ(settings_manager.MaybeSendAck(), 2u); - EXPECT_EQ(settings_manager.MaybeSendAck(), 0u); -} - } // namespace grpc_core int main(int argc, char** argv) { diff --git a/test/core/transport/chttp2/http2_transport_test.cc b/test/core/transport/chttp2/http2_transport_test.cc index 38071611ec..77fdad458b 100644 --- a/test/core/transport/chttp2/http2_transport_test.cc +++ b/test/core/transport/chttp2/http2_transport_test.cc @@ -161,172 +161,6 @@ TEST(Http2CommonTransportTest, TestReadChannelArgs) { EXPECT_EQ(settings2.allow_security_frame(), false); } -TEST(SettingsPromiseManagerTest1, MaybeGetSettingsAndSettingsAckFramesIdle) { - // Tests that in idle state, first call to - // MaybeGetSettingsAndSettingsAckFrames sends initial settings, and second - // call does nothing. - chttp2::TransportFlowControl transport_flow_control( - /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, - /*memory_owner=*/nullptr); - RefCountedPtr timeout_manager = - MakeRefCounted(); - SliceBuffer output_buf; - // We add "hello" to output_buf to ensure that - // MaybeGetSettingsAndSettingsAckFrames appends to it and does not overwrite - // it, i.e. the original contents of output_buf are not erased. - output_buf.Append(Slice::FromCopiedString("hello")); - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - timeout_manager->TestOnlyTimeoutWaiterSpawned(); - ASSERT_THAT(output_buf.JoinIntoString(), ::testing::StartsWith("hello")); - EXPECT_GT(output_buf.Length(), 5); - output_buf.Clear(); - output_buf.Append(Slice::FromCopiedString("hello")); - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - EXPECT_EQ(output_buf.Length(), 5); - EXPECT_EQ(output_buf.JoinIntoString(), "hello"); -} - -TEST(SettingsPromiseManagerTest1, - MaybeGetSettingsAndSettingsAckFramesMultipleAcks) { - // If multiple settings frames are received then multiple ACKs should be sent. - chttp2::TransportFlowControl transport_flow_control( - /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, - /*memory_owner=*/nullptr); - RefCountedPtr timeout_manager = - MakeRefCounted(); - SliceBuffer output_buf; - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - timeout_manager->TestOnlyTimeoutWaiterSpawned(); - output_buf.Clear(); - output_buf.Append(Slice::FromCopiedString("hello")); - for (int i = 0; i < 5; ++i) { - timeout_manager->BufferPeerSettings( - {{Http2Settings::kMaxConcurrentStreamsWireId, 100}}); - } - - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - - SliceBuffer expected_buf; - expected_buf.Append(Slice::FromCopiedString("hello")); - for (int i = 0; i < 5; ++i) { - Http2SettingsFrame settings; - settings.ack = true; - Http2Frame frame(settings); - Serialize(absl::Span(&frame, 1), expected_buf); - } - EXPECT_EQ(output_buf.Length(), expected_buf.Length()); - EXPECT_EQ(output_buf.JoinIntoString(), expected_buf.JoinIntoString()); -} - -TEST(SettingsPromiseManagerTest1, - MaybeGetSettingsAndSettingsAckFramesAfterAckAndChange) { - // Tests that after initial settings are sent and ACKed, no frame is sent. If - // settings are changed, a new SETTINGS frame with diff is sent. - chttp2::TransportFlowControl transport_flow_control( - /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, - /*memory_owner=*/nullptr); - RefCountedPtr timeout_manager = - MakeRefCounted(); - const uint32_t kSetMaxFrameSize = 16385; - SliceBuffer output_buf; - // We add "hello" to output_buf to ensure that - // MaybeGetSettingsAndSettingsAckFrames appends to it and does not overwrite - // it, i.e. the original contents of output_buf are not erased. - output_buf.Append(Slice::FromCopiedString("hello")); - // Initial settings - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - timeout_manager->TestOnlyTimeoutWaiterSpawned(); - ASSERT_THAT(output_buf.JoinIntoString(), ::testing::StartsWith("hello")); - EXPECT_GT(output_buf.Length(), 5); - // Ack settings - EXPECT_TRUE(timeout_manager->OnSettingsAckReceived()); - output_buf.Clear(); - output_buf.Append(Slice::FromCopiedString("hello")); - // No changes - no frames - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - EXPECT_EQ(output_buf.Length(), 5); - EXPECT_EQ(output_buf.JoinIntoString(), "hello"); - output_buf.Clear(); - // Change settings - timeout_manager->mutable_local().SetMaxFrameSize(kSetMaxFrameSize); - output_buf.Append(Slice::FromCopiedString("hello")); - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - timeout_manager->TestOnlyTimeoutWaiterSpawned(); - // Check frame - Http2SettingsFrame expected_settings; - expected_settings.ack = false; - expected_settings.settings.push_back( - {Http2Settings::kMaxFrameSizeWireId, kSetMaxFrameSize}); - Http2Frame expected_frame(expected_settings); - SliceBuffer expected_buf; - expected_buf.Append(Slice::FromCopiedString("hello")); - Serialize(absl::Span(&expected_frame, 1), expected_buf); - EXPECT_EQ(output_buf.Length(), expected_buf.Length()); - EXPECT_EQ(output_buf.JoinIntoString(), expected_buf.JoinIntoString()); - - // We set SetMaxFrameSize to the same value as previous value. - // The Diff will be zero, in this case a new SETTINGS frame must not be sent. - timeout_manager->mutable_local().SetMaxFrameSize(kSetMaxFrameSize); - output_buf.Clear(); - output_buf.Append(Slice::FromCopiedString("hello")); - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - EXPECT_EQ(output_buf.Length(), 5); - EXPECT_EQ(output_buf.JoinIntoString(), "hello"); -} - -TEST(SettingsPromiseManagerTest1, MaybeGetSettingsAndSettingsAckFramesWithAck) { - // Tests that if we need to send initial settings and also ACK received - // settings, both frames are sent. - chttp2::TransportFlowControl transport_flow_control( - /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, - /*memory_owner=*/nullptr); - RefCountedPtr timeout_manager = - MakeRefCounted(); - SliceBuffer output_buf; - // We add "hello" to output_buf to ensure that - // MaybeGetSettingsAndSettingsAckFrames appends to it and does not overwrite - // it, i.e. the original contents of output_buf are not erased. - output_buf.Append(Slice::FromCopiedString("hello")); - timeout_manager->BufferPeerSettings( - {{Http2Settings::kMaxConcurrentStreamsWireId, 100}}); - timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, - output_buf); - EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); - timeout_manager->TestOnlyTimeoutWaiterSpawned(); - Http2SettingsFrame expected_settings; - expected_settings.ack = false; - timeout_manager->mutable_local().Diff( - true, Http2Settings(), [&](uint16_t key, uint32_t value) { - expected_settings.settings.push_back({key, value}); - }); - Http2SettingsFrame expected_settings_ack; - expected_settings_ack.ack = true; - SliceBuffer expected_buf; - expected_buf.Append(Slice::FromCopiedString("hello")); - std::vector frames; - frames.emplace_back(expected_settings); - frames.emplace_back(expected_settings_ack); - Serialize(absl::MakeSpan(frames), expected_buf); - EXPECT_EQ(output_buf.Length(), expected_buf.Length()); - EXPECT_EQ(output_buf.JoinIntoString(), expected_buf.JoinIntoString()); -} - /////////////////////////////////////////////////////////////////////////////// // Flow control helpers tests diff --git a/test/core/transport/chttp2/settings_timeout_manager_test.cc b/test/core/transport/chttp2/settings_timeout_manager_test.cc index d8901fa6b9..0781a5dd52 100644 --- a/test/core/transport/chttp2/settings_timeout_manager_test.cc +++ b/test/core/transport/chttp2/settings_timeout_manager_test.cc @@ -24,8 +24,11 @@ #include #include #include +#include +#include "src/core/ext/transport/chttp2/transport/flow_control.h" #include "src/core/ext/transport/chttp2/transport/frame.h" +#include "src/core/ext/transport/chttp2/transport/http2_settings.h" #include "src/core/ext/transport/chttp2/transport/http2_settings_promises.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/event_engine/default_event_engine.h" @@ -36,13 +39,18 @@ #include "src/core/lib/promise/try_join.h" #include "src/core/lib/promise/try_seq.h" #include "src/core/lib/resource_quota/arena.h" +#include "src/core/lib/slice/slice.h" +#include "src/core/lib/slice/slice_buffer.h" #include "src/core/util/notification.h" #include "src/core/util/orphanable.h" +#include "src/core/util/ref_counted_ptr.h" #include "src/core/util/time.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/log/log.h" #include "absl/status/status.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" namespace grpc_core { namespace http2 { @@ -261,6 +269,172 @@ TEST_F(SettingsPromiseManagerTest, TimeoutOneSetting) { notification2.WaitForNotification(); } +TEST(SettingsPromiseManagerTest1, MaybeGetSettingsAndSettingsAckFramesIdle) { + // Tests that in idle state, first call to + // MaybeGetSettingsAndSettingsAckFrames sends initial settings, and second + // call does nothing. + chttp2::TransportFlowControl transport_flow_control( + /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, + /*memory_owner=*/nullptr); + RefCountedPtr timeout_manager = + MakeRefCounted(); + SliceBuffer output_buf; + // We add "hello" to output_buf to ensure that + // MaybeGetSettingsAndSettingsAckFrames appends to it and does not overwrite + // it, i.e. the original contents of output_buf are not erased. + output_buf.Append(Slice::FromCopiedString("hello")); + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + timeout_manager->TestOnlyTimeoutWaiterSpawned(); + ASSERT_THAT(output_buf.JoinIntoString(), ::testing::StartsWith("hello")); + EXPECT_GT(output_buf.Length(), 5); + output_buf.Clear(); + output_buf.Append(Slice::FromCopiedString("hello")); + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + EXPECT_EQ(output_buf.Length(), 5); + EXPECT_EQ(output_buf.JoinIntoString(), "hello"); +} + +TEST(SettingsPromiseManagerTest1, + MaybeGetSettingsAndSettingsAckFramesMultipleAcks) { + // If multiple settings frames are received then multiple ACKs should be sent. + chttp2::TransportFlowControl transport_flow_control( + /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, + /*memory_owner=*/nullptr); + RefCountedPtr timeout_manager = + MakeRefCounted(); + SliceBuffer output_buf; + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + timeout_manager->TestOnlyTimeoutWaiterSpawned(); + output_buf.Clear(); + output_buf.Append(Slice::FromCopiedString("hello")); + for (int i = 0; i < 5; ++i) { + timeout_manager->BufferPeerSettings( + {{Http2Settings::kMaxConcurrentStreamsWireId, 100}}); + } + + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + + SliceBuffer expected_buf; + expected_buf.Append(Slice::FromCopiedString("hello")); + for (int i = 0; i < 5; ++i) { + Http2SettingsFrame settings; + settings.ack = true; + Http2Frame frame(settings); + Serialize(absl::Span(&frame, 1), expected_buf); + } + EXPECT_EQ(output_buf.Length(), expected_buf.Length()); + EXPECT_EQ(output_buf.JoinIntoString(), expected_buf.JoinIntoString()); +} + +TEST(SettingsPromiseManagerTest1, + MaybeGetSettingsAndSettingsAckFramesAfterAckAndChange) { + // Tests that after initial settings are sent and ACKed, no frame is sent. If + // settings are changed, a new SETTINGS frame with diff is sent. + chttp2::TransportFlowControl transport_flow_control( + /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, + /*memory_owner=*/nullptr); + RefCountedPtr timeout_manager = + MakeRefCounted(); + const uint32_t kSetMaxFrameSize = 16385; + SliceBuffer output_buf; + // We add "hello" to output_buf to ensure that + // MaybeGetSettingsAndSettingsAckFrames appends to it and does not overwrite + // it, i.e. the original contents of output_buf are not erased. + output_buf.Append(Slice::FromCopiedString("hello")); + // Initial settings + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + timeout_manager->TestOnlyTimeoutWaiterSpawned(); + ASSERT_THAT(output_buf.JoinIntoString(), ::testing::StartsWith("hello")); + EXPECT_GT(output_buf.Length(), 5); + // Ack settings + EXPECT_TRUE(timeout_manager->OnSettingsAckReceived()); + output_buf.Clear(); + output_buf.Append(Slice::FromCopiedString("hello")); + // No changes - no frames + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + EXPECT_EQ(output_buf.Length(), 5); + EXPECT_EQ(output_buf.JoinIntoString(), "hello"); + output_buf.Clear(); + // Change settings + timeout_manager->mutable_local().SetMaxFrameSize(kSetMaxFrameSize); + output_buf.Append(Slice::FromCopiedString("hello")); + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + timeout_manager->TestOnlyTimeoutWaiterSpawned(); + // Check frame + Http2SettingsFrame expected_settings; + expected_settings.ack = false; + expected_settings.settings.push_back( + {Http2Settings::kMaxFrameSizeWireId, kSetMaxFrameSize}); + Http2Frame expected_frame(expected_settings); + SliceBuffer expected_buf; + expected_buf.Append(Slice::FromCopiedString("hello")); + Serialize(absl::Span(&expected_frame, 1), expected_buf); + EXPECT_EQ(output_buf.Length(), expected_buf.Length()); + EXPECT_EQ(output_buf.JoinIntoString(), expected_buf.JoinIntoString()); + + // We set SetMaxFrameSize to the same value as previous value. + // The Diff will be zero, in this case a new SETTINGS frame must not be sent. + timeout_manager->mutable_local().SetMaxFrameSize(kSetMaxFrameSize); + output_buf.Clear(); + output_buf.Append(Slice::FromCopiedString("hello")); + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_FALSE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + EXPECT_EQ(output_buf.Length(), 5); + EXPECT_EQ(output_buf.JoinIntoString(), "hello"); +} + +TEST(SettingsPromiseManagerTest1, MaybeGetSettingsAndSettingsAckFramesWithAck) { + // Tests that if we need to send initial settings and also ACK received + // settings, both frames are sent. + chttp2::TransportFlowControl transport_flow_control( + /*name=*/"TestFlowControl", /*enable_bdp_probe=*/false, + /*memory_owner=*/nullptr); + RefCountedPtr timeout_manager = + MakeRefCounted(); + SliceBuffer output_buf; + // We add "hello" to output_buf to ensure that + // MaybeGetSettingsAndSettingsAckFrames appends to it and does not overwrite + // it, i.e. the original contents of output_buf are not erased. + output_buf.Append(Slice::FromCopiedString("hello")); + timeout_manager->BufferPeerSettings( + {{Http2Settings::kMaxConcurrentStreamsWireId, 100}}); + timeout_manager->MaybeGetSettingsAndSettingsAckFrames(transport_flow_control, + output_buf); + EXPECT_TRUE(timeout_manager->ShouldSpawnWaitForSettingsTimeout()); + timeout_manager->TestOnlyTimeoutWaiterSpawned(); + Http2SettingsFrame expected_settings; + expected_settings.ack = false; + timeout_manager->mutable_local().Diff( + true, Http2Settings(), [&](uint16_t key, uint32_t value) { + expected_settings.settings.push_back({key, value}); + }); + Http2SettingsFrame expected_settings_ack; + expected_settings_ack.ack = true; + SliceBuffer expected_buf; + expected_buf.Append(Slice::FromCopiedString("hello")); + std::vector frames; + frames.emplace_back(expected_settings); + frames.emplace_back(expected_settings_ack); + Serialize(absl::MakeSpan(frames), expected_buf); + EXPECT_EQ(output_buf.Length(), expected_buf.Length()); + EXPECT_EQ(output_buf.JoinIntoString(), expected_buf.JoinIntoString()); +} + } // namespace testing } // namespace http2