[http2] Add experiment to modify behavior of GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA to throttle pings instead of blocking

include/grpc/impl/channel_arg_names.h

@@ -114,9 +114,11 @@
   "grpc.server_max_unrequested_time_in_server"
 /** Channel arg to override the http2 :scheme header */
 #define GRPC_ARG_HTTP2_SCHEME "grpc.http2_scheme"
-/** How many pings can the client send before needing to send a
-   data/header frame? (0 indicates that an infinite number of
-   pings can be sent without sending a data frame or header frame) */
+/** How many pings can the client send before needing to send a data/header
+   frame? (0 indicates that an infinite number of pings can be sent without
+   sending a data frame or header frame).
+   If experiment "max_pings_wo_data_throttle" is enabled, instead of pings being
+   completely blocked, they are throttled. */
 #define GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA \
   "grpc.http2.max_pings_without_data"
 /** How many misbehaving pings the server can bear before sending goaway and

src/core/ext/transport/chttp2/transport/ping_rate_policy.cc

@@ -36,16 +36,18 @@
 namespace grpc_core {
 
 namespace {
-int g_default_max_pings_without_data = 2;
+int g_default_max_pings_without_data_sent = 2;
+constexpr Duration kThrottleIntervalWithoutDataSent = Duration::Minutes(1);
 absl::optional<int> g_default_max_inflight_pings;
 }  // namespace
 
 Chttp2PingRatePolicy::Chttp2PingRatePolicy(const ChannelArgs& args,
                                            bool is_client)
-    : max_pings_without_data_(
+    : max_pings_without_data_sent_(
           is_client
-              ? std::max(0, args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA)
-                                .value_or(g_default_max_pings_without_data))
+              ? std::max(0,
+                         args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA)
+                             .value_or(g_default_max_pings_without_data_sent))
               : 0),
       // Configuration via channel arg dominates, otherwise if the multiping
       // experiment is enabled we use 100, otherwise 1.
@@ -55,18 +57,15 @@ Chttp2PingRatePolicy::Chttp2PingRatePolicy(const ChannelArgs& args,
                               IsMultipingEnabled() ? 100 : 1)))) {}
 
 void Chttp2PingRatePolicy::SetDefaults(const ChannelArgs& args) {
-  g_default_max_pings_without_data =
+  g_default_max_pings_without_data_sent =
       std::max(0, args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA)
-                      .value_or(g_default_max_pings_without_data));
+                      .value_or(g_default_max_pings_without_data_sent));
   g_default_max_inflight_pings = args.GetInt(GRPC_ARG_HTTP2_MAX_INFLIGHT_PINGS);
 }
 
 Chttp2PingRatePolicy::RequestSendPingResult
 Chttp2PingRatePolicy::RequestSendPing(Duration next_allowed_ping_interval,
                                       size_t inflight_pings) const {
-  if (max_pings_without_data_ != 0 && pings_before_data_required_ == 0) {
-    return TooManyRecentPings{};
-  }
   if (max_inflight_pings_ > 0 &&
       inflight_pings > static_cast<size_t>(max_inflight_pings_)) {
     return TooManyRecentPings{};
@@ -78,26 +77,43 @@ Chttp2PingRatePolicy::RequestSendPing(Duration next_allowed_ping_interval,
     return TooSoon{next_allowed_ping_interval, last_ping_sent_time_,
                    next_allowed_ping - now};
   }
+  // Throttle pings to 1 minute if we haven't sent any data recently
+  if (max_pings_without_data_sent_ != 0 &&
+      pings_before_data_sending_required_ == 0) {
+    if (IsMaxPingsWoDataThrottleEnabled()) {
+      const Timestamp next_allowed_ping =
+          last_ping_sent_time_ + kThrottleIntervalWithoutDataSent;
+      if (next_allowed_ping > now) {
+        return TooSoon{kThrottleIntervalWithoutDataSent, last_ping_sent_time_,
+                       next_allowed_ping - now};
+      }
+    } else {
+      return TooManyRecentPings{};
+    }
+  }
+
   return SendGranted{};
 }
 
 void Chttp2PingRatePolicy::SentPing() {
   last_ping_sent_time_ = Timestamp::Now();
-  if (pings_before_data_required_) --pings_before_data_required_;
+  if (pings_before_data_sending_required_) {
+    --pings_before_data_sending_required_;
+  }
 }
 
 void Chttp2PingRatePolicy::ReceivedDataFrame() {
   last_ping_sent_time_ = Timestamp::InfPast();
 }
 
 void Chttp2PingRatePolicy::ResetPingsBeforeDataRequired() {
-  pings_before_data_required_ = max_pings_without_data_;
+  pings_before_data_sending_required_ = max_pings_without_data_sent_;
 }
 
 std::string Chttp2PingRatePolicy::GetDebugString() const {
   return absl::StrCat(
-      "max_pings_without_data: ", max_pings_without_data_,
-      ", pings_before_data_required: ", pings_before_data_required_,
+      "max_pings_without_data: ", max_pings_without_data_sent_,
+      ", pings_before_data_required: ", pings_before_data_sending_required_,
       ", last_ping_sent_time_: ", last_ping_sent_time_.ToString());
 }
 

src/core/ext/transport/chttp2/transport/ping_rate_policy.h

@@ -70,13 +70,14 @@ class Chttp2PingRatePolicy {
   void ReceivedDataFrame();
   std::string GetDebugString() const;
 
-  int TestOnlyMaxPingsWithoutData() const { return max_pings_without_data_; }
+  int TestOnlyMaxPingsWithoutData() const {
+    return max_pings_without_data_sent_;
+  }
 
  private:
-  const int max_pings_without_data_;
+  const int max_pings_without_data_sent_;
   const int max_inflight_pings_;
-  // No pings allowed before receiving a header or data frame.
-  int pings_before_data_required_ = 0;
+  int pings_before_data_sending_required_ = 0;
   Timestamp last_ping_sent_time_ = Timestamp::InfPast();
 };
 

src/core/lib/experiments/experiments.yaml

@@ -114,6 +114,14 @@
   owner: yashkt@google.com
   test_tags: []
   allow_in_fuzzing_config: false
+- name: max_pings_wo_data_throttle
+  description:
+    Experiment to throttle pings to a period of 1 min when
+    GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of
+    completely blocking).
+  expiry: 2024/12/31
+  owner: yashkt@google.com
+  test_tags: []
 - name: monitoring_experiment
   description: Placeholder experiment to prove/disprove our monitoring is working
   expiry: never-ever

src/core/lib/experiments/rollouts.yaml

@@ -83,6 +83,8 @@
   default: false
 - name: keepalive_server_fix
   default: false
+- name: max_pings_wo_data_throttle
+  default: false
 - name: monitoring_experiment
   default: true
 - name: peer_state_based_framing

test/core/end2end/tests/bad_ping.cc

@@ -87,6 +87,9 @@ CORE_END2END_TEST(RetryHttp2Test, BadPing) {
 // Try sending more pings than server allows, but server should be fine because
 // max_pings_without_data should limit pings sent out on wire.
 CORE_END2END_TEST(RetryHttp2Test, PingsWithoutData) {
+  if (IsMaxPingsWoDataThrottleEnabled()) {
+    GTEST_SKIP() << "pings are not limited if this experiment is enabled";
+  }
   // Only allow MAX_PING_STRIKES pings without data (DATA/HEADERS/WINDOW_UPDATE)
   // so that the transport will throttle the excess pings.
   InitClient(ChannelArgs()

test/core/transport/chttp2/ping_rate_policy_test.cc

@@ -19,6 +19,9 @@
 
 #include "gtest/gtest.h"
 
+#include "src/core/lib/experiments/experiments.h"
+#include "test/core/util/test_config.h"
+
 namespace grpc_core {
 namespace {
 
@@ -47,6 +50,10 @@ TEST(PingRatePolicy, ServerCanSendAtStart) {
 }
 
 TEST(PingRatePolicy, ClientBlockedUntilDataSent) {
+  if (IsMaxPingsWoDataThrottleEnabled()) {
+    GTEST_SKIP()
+        << "Pings are not blocked if max_pings_wo_data_throttle is enabled.";
+  }
   Chttp2PingRatePolicy policy{ChannelArgs(), true};
   EXPECT_EQ(policy.RequestSendPing(Duration::Milliseconds(10), 0),
             TooManyRecentPings());
@@ -59,6 +66,35 @@ TEST(PingRatePolicy, ClientBlockedUntilDataSent) {
   EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), TooManyRecentPings());
 }
 
+TEST(PingRatePolicy, ClientThrottledUntilDataSent) {
+  if (!IsMaxPingsWoDataThrottleEnabled()) {
+    GTEST_SKIP()
+        << "Throttling behavior is enabled with max_pings_wo_data_throttle.";
+  }
+  Chttp2PingRatePolicy policy{ChannelArgs(), true};
+  // First ping is allowed.
+  EXPECT_EQ(policy.RequestSendPing(Duration::Milliseconds(10), 0),
+            SendGranted());
+  policy.SentPing();
+  // Second ping is throttled since no data has been sent.
+  auto result = policy.RequestSendPing(Duration::Zero(), 0);
+  EXPECT_TRUE(absl::holds_alternative<Chttp2PingRatePolicy::TooSoon>(result));
+  EXPECT_EQ(absl::get<Chttp2PingRatePolicy::TooSoon>(result).wait,
+            Duration::Minutes(1));
+  policy.ResetPingsBeforeDataRequired();
+  // After resetting pings before data required (data sent), we can send pings
+  // without being throttled.
+  EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), SendGranted());
+  policy.SentPing();
+  EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), SendGranted());
+  policy.SentPing();
+  // After reaching limit, we are throttled again.
+  result = policy.RequestSendPing(Duration::Zero(), 0);
+  EXPECT_TRUE(absl::holds_alternative<Chttp2PingRatePolicy::TooSoon>(result));
+  EXPECT_EQ(absl::get<Chttp2PingRatePolicy::TooSoon>(result).wait,
+            Duration::Minutes(1));
+}
+
 TEST(PingRatePolicy, RateThrottlingWorks) {
   Chttp2PingRatePolicy policy{ChannelArgs(), false};
   // Observe that we can fail if we send in a tight loop
@@ -86,5 +122,9 @@ TEST(PingRatePolicy, TooManyPingsInflightBlocksSendingPings) {
 
 int main(int argc, char** argv) {
   ::testing::InitGoogleTest(&argc, argv);
-  return RUN_ALL_TESTS();
+  grpc::testing::TestEnvironment env(&argc, argv);
+  grpc_init();
+  int result = RUN_ALL_TESTS();
+  grpc_shutdown();
+  return result;
 }