From 039d7f51d61eb8053ab05447721975562b5333c1 Mon Sep 17 00:00:00 2001 From: Akshit Patel Date: Fri, 19 Dec 2025 00:37:24 -0800 Subject: [PATCH] [PH2][ChanelArgs] Support AckPing PiperOrigin-RevId: 846606868 --- .../transport/http2_client_transport.cc | 39 ++++++++++++------- .../chttp2/transport/http2_client_transport.h | 1 + .../chttp2/transport/http2_transport.cc | 4 ++ .../chttp2/transport/http2_transport.h | 5 ++- 4 files changed, 34 insertions(+), 15 deletions(-) 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 bc6786d565..9e3201e3d3 100644 --- a/src/core/ext/transport/chttp2/transport/http2_client_transport.cc +++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.cc @@ -508,20 +508,30 @@ auto Http2ClientTransport::ProcessHttp2PingFrame(Http2PingFrame frame) { return self->AckPing(opaque); }, [self = RefAsSubclass(), opaque = frame.opaque]() { - // TODO(akshitpatel) : [PH2][P2] : Have a counter to track number of - // pending induced frames (Ping/Settings Ack). This is to ensure that - // if write is taking a long time, we can stop reads and prioritize - // writes. - // RFC9113: PING responses SHOULD be given higher priority than any - // other frame. - self->ping_manager_->AddPendingPingAck(opaque); - // TODO(akshitpatel) : [PH2][P2] : This is done assuming that the other - // ProcessFrame promises may return stream or connection failures. If - // this does not turn out to be true, consider returning absl::Status - // here. - return Map(self->TriggerWriteCycle(), [](absl::Status status) { - return ToHttpOkOrConnError(status); - }); + return If( + self->test_only_ack_pings_, + [self, opaque]() { + // TODO(akshitpatel) : [PH2][P2] : Have a counter to track number + // of pending induced frames (Ping/Settings Ack). This is to + // ensure that if write is taking a long time, we can stop reads + // and prioritize writes. RFC9113: PING responses SHOULD be given + // higher priority than any other frame. + self->ping_manager_->AddPendingPingAck(opaque); + // TODO(akshitpatel) : [PH2][P2] : This is done assuming that the + // other ProcessFrame promises may return stream or connection + // failures. If this does not turn out to be true, consider + // returning absl::Status here. + return Map(self->TriggerWriteCycle(), [](absl::Status status) { + return ToHttpOkOrConnError(status); + }); + }, + []() { + GRPC_HTTP2_CLIENT_DLOG + << "Http2ClientTransport ProcessHttp2PingFrame " + "test_only_ack_pings_ is false. Ignoring the ping " + "request."; + return Immediate(Http2Status::Ok()); + }); })); } @@ -1437,6 +1447,7 @@ void Http2ClientTransport::ReadChannelArgs(const ChannelArgs& channel_args, keepalive_permit_without_calls_ = args.keepalive_permit_without_calls; enable_preferred_rx_crypto_frame_advertisement_ = args.enable_preferred_rx_crypto_frame_advertisement; + test_only_ack_pings_ = args.test_only_ack_pings; if (args.initial_sequence_number > 0) { next_stream_id_ = args.initial_sequence_number; 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 0f6e4c8a96..a272f8b601 100644 --- a/src/core/ext/transport/chttp2/transport/http2_client_transport.h +++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.h @@ -533,6 +533,7 @@ class Http2ClientTransport final : public ClientTransport, // Duration between two consecutive keepalive pings. Duration keepalive_time_; + bool test_only_ack_pings_; std::optional ping_manager_; std::optional keepalive_manager_; diff --git a/src/core/ext/transport/chttp2/transport/http2_transport.cc b/src/core/ext/transport/chttp2/transport/http2_transport.cc index 6e53864390..497a7d013b 100644 --- a/src/core/ext/transport/chttp2/transport/http2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/http2_transport.cc @@ -58,6 +58,7 @@ constexpr Duration kDefaultPingTimeout = Duration::Minutes(1); constexpr Duration kDefaultKeepaliveTimeout = Duration::Seconds(20); constexpr bool kDefaultKeepalivePermitWithoutCalls = false; constexpr bool kDefaultEnablePreferredRxCryptoFrameAdvertisement = false; +constexpr bool kDefaultAckPings = true; constexpr Duration kClientKeepaliveTime = Duration::Infinity(); @@ -134,6 +135,9 @@ void ReadChannelArgs(const ChannelArgs& channel_args, args.initial_sequence_number = -1; } + args.test_only_ack_pings = + channel_args.GetBool("grpc.http2.ack_pings").value_or(kDefaultAckPings); + GRPC_HTTP2_COMMON_DLOG << "ChannelArgs: " << args.DebugString(); } diff --git a/src/core/ext/transport/chttp2/transport/http2_transport.h b/src/core/ext/transport/chttp2/transport/http2_transport.h index 1d12097edb..f9e9354781 100644 --- a/src/core/ext/transport/chttp2/transport/http2_transport.h +++ b/src/core/ext/transport/chttp2/transport/http2_transport.h @@ -118,6 +118,8 @@ struct TransportChannelArgs { Duration settings_timeout; bool keepalive_permit_without_calls; bool enable_preferred_rx_crypto_frame_advertisement; + // This is used to test peer behaviour when we never send a ping ack. + bool test_only_ack_pings; uint32_t max_header_list_size_soft_limit; int max_usable_hpack_table_size; int initial_sequence_number; @@ -133,7 +135,8 @@ struct TransportChannelArgs { enable_preferred_rx_crypto_frame_advertisement, " max_header_list_size_soft_limit: ", max_header_list_size_soft_limit, " max_usable_hpack_table_size: ", max_usable_hpack_table_size, - " initial_sequence_number: ", initial_sequence_number); + " initial_sequence_number: ", initial_sequence_number, + " test_only_ack_pings: ", test_only_ack_pings); } }; -- 2.43.0