[http2] Add experiment to modify behavior of GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA to throttle pings instead of blocking (#36374)

Implements https://github.com/grpc/proposal/pull/429

Currently, the behavior of `GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA` blocks more pings from being sent if we are sending too many pings without a data/header frame being sent as well. The original intention of this channel arg was to play nice with proxies that have restrictive settings when it comes to pings. This causes awkwardness when configuring keepalive pings for transports with long lived streams with sparse communication. In such a case, gRPC Core would stop sending keepalive pings since no data/header frame is being sent, resulting in a situation where we are unable to detect whether the transport is alive or not.

This change adds an experiment "max_pings_wo_data_throttle" to modify the behavior of `GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA` to throttle pings to a frequency of 1 minute instead of completely blocking pings when too many pings have been sent without data/header frames.

Closes #36374

COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36374 from yashykt:ThrottlePings b5bd42a019
PiperOrigin-RevId: 638110795

bazel/experiments.bzl

@@ -28,6 +28,7 @@ EXPERIMENT_ENABLES = {
     "http2_stats_fix": "http2_stats_fix",
     "keepalive_fix": "keepalive_fix",
     "keepalive_server_fix": "keepalive_server_fix",
+    "max_pings_wo_data_throttle": "max_pings_wo_data_throttle",
     "monitoring_experiment": "monitoring_experiment",
     "multiping": "multiping",
     "peer_state_based_framing": "peer_state_based_framing",

include/grpc/impl/channel_arg_names.h

@@ -111,9 +111,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
+/** How many pings can the client send before needing to send a data/header
-   data/header frame? (0 indicates that an infinite number of
+   frame? (0 indicates that an infinite number of pings can be sent without
-   pings can be sent without sending a data frame or header frame) */
+   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

@@ -35,16 +35,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)
+              ? std::max(0,
-                                .value_or(g_default_max_pings_without_data))
+                         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.
@@ -54,18 +56,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{};
@@ -77,12 +76,29 @@ 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_ > 0) {
+    --pings_before_data_sending_required_;
+  }
 }
 
 void Chttp2PingRatePolicy::ReceivedDataFrame() {
@@ -90,13 +106,13 @@ void Chttp2PingRatePolicy::ReceivedDataFrame() {
 }
 
 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_,
+      "max_pings_without_data: ", max_pings_without_data_sent_,
-      ", pings_before_data_required: ", pings_before_data_required_,
+      ", 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_sending_required_ = 0;
-  int pings_before_data_required_ = 0;
   Timestamp last_ping_sent_time_ = Timestamp::InfPast();
 };
 

src/core/lib/experiments/experiments.cc

@@ -59,6 +59,11 @@ const char* const description_keepalive_server_fix =
     "Allows overriding keepalive_permit_without_calls for servers. Refer "
     "https://github.com/grpc/grpc/pull/33917 for more information.";
 const char* const additional_constraints_keepalive_server_fix = "{}";
+const char* const description_max_pings_wo_data_throttle =
+    "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).";
+const char* const additional_constraints_max_pings_wo_data_throttle = "{}";
 const char* const description_monitoring_experiment =
     "Placeholder experiment to prove/disprove our monitoring is working";
 const char* const additional_constraints_monitoring_experiment = "{}";
@@ -157,6 +162,9 @@ const ExperimentMetadata g_experiment_metadata[] = {
      additional_constraints_keepalive_fix, nullptr, 0, false, false},
     {"keepalive_server_fix", description_keepalive_server_fix,
      additional_constraints_keepalive_server_fix, nullptr, 0, false, false},
+    {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle,
+     additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false,
+     true},
     {"monitoring_experiment", description_monitoring_experiment,
      additional_constraints_monitoring_experiment, nullptr, 0, true, true},
     {"multiping", description_multiping, additional_constraints_multiping,
@@ -241,6 +249,11 @@ const char* const description_keepalive_server_fix =
     "Allows overriding keepalive_permit_without_calls for servers. Refer "
     "https://github.com/grpc/grpc/pull/33917 for more information.";
 const char* const additional_constraints_keepalive_server_fix = "{}";
+const char* const description_max_pings_wo_data_throttle =
+    "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).";
+const char* const additional_constraints_max_pings_wo_data_throttle = "{}";
 const char* const description_monitoring_experiment =
     "Placeholder experiment to prove/disprove our monitoring is working";
 const char* const additional_constraints_monitoring_experiment = "{}";
@@ -339,6 +352,9 @@ const ExperimentMetadata g_experiment_metadata[] = {
      additional_constraints_keepalive_fix, nullptr, 0, false, false},
     {"keepalive_server_fix", description_keepalive_server_fix,
      additional_constraints_keepalive_server_fix, nullptr, 0, false, false},
+    {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle,
+     additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false,
+     true},
     {"monitoring_experiment", description_monitoring_experiment,
      additional_constraints_monitoring_experiment, nullptr, 0, true, true},
     {"multiping", description_multiping, additional_constraints_multiping,
@@ -423,6 +439,11 @@ const char* const description_keepalive_server_fix =
     "Allows overriding keepalive_permit_without_calls for servers. Refer "
     "https://github.com/grpc/grpc/pull/33917 for more information.";
 const char* const additional_constraints_keepalive_server_fix = "{}";
+const char* const description_max_pings_wo_data_throttle =
+    "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).";
+const char* const additional_constraints_max_pings_wo_data_throttle = "{}";
 const char* const description_monitoring_experiment =
     "Placeholder experiment to prove/disprove our monitoring is working";
 const char* const additional_constraints_monitoring_experiment = "{}";
@@ -521,6 +542,9 @@ const ExperimentMetadata g_experiment_metadata[] = {
      additional_constraints_keepalive_fix, nullptr, 0, false, false},
     {"keepalive_server_fix", description_keepalive_server_fix,
      additional_constraints_keepalive_server_fix, nullptr, 0, false, false},
+    {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle,
+     additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false,
+     true},
     {"monitoring_experiment", description_monitoring_experiment,
      additional_constraints_monitoring_experiment, nullptr, 0, true, true},
     {"multiping", description_multiping, additional_constraints_multiping,

src/core/lib/experiments/experiments.h

@@ -70,6 +70,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; }
 inline bool IsHttp2StatsFixEnabled() { return true; }
 inline bool IsKeepaliveFixEnabled() { return false; }
 inline bool IsKeepaliveServerFixEnabled() { return false; }
+inline bool IsMaxPingsWoDataThrottleEnabled() { return false; }
 #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT
 inline bool IsMonitoringExperimentEnabled() { return true; }
 inline bool IsMultipingEnabled() { return false; }
@@ -107,6 +108,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; }
 inline bool IsHttp2StatsFixEnabled() { return true; }
 inline bool IsKeepaliveFixEnabled() { return false; }
 inline bool IsKeepaliveServerFixEnabled() { return false; }
+inline bool IsMaxPingsWoDataThrottleEnabled() { return false; }
 #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT
 inline bool IsMonitoringExperimentEnabled() { return true; }
 inline bool IsMultipingEnabled() { return false; }
@@ -144,6 +146,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; }
 inline bool IsHttp2StatsFixEnabled() { return true; }
 inline bool IsKeepaliveFixEnabled() { return false; }
 inline bool IsKeepaliveServerFixEnabled() { return false; }
+inline bool IsMaxPingsWoDataThrottleEnabled() { return false; }
 #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT
 inline bool IsMonitoringExperimentEnabled() { return true; }
 inline bool IsMultipingEnabled() { return false; }
@@ -180,6 +183,7 @@ enum ExperimentIds {
   kExperimentIdHttp2StatsFix,
   kExperimentIdKeepaliveFix,
   kExperimentIdKeepaliveServerFix,
+  kExperimentIdMaxPingsWoDataThrottle,
   kExperimentIdMonitoringExperiment,
   kExperimentIdMultiping,
   kExperimentIdPeerStateBasedFraming,
@@ -242,6 +246,10 @@ inline bool IsKeepaliveFixEnabled() {
 inline bool IsKeepaliveServerFixEnabled() {
   return IsExperimentEnabled(kExperimentIdKeepaliveServerFix);
 }
+#define GRPC_EXPERIMENT_IS_INCLUDED_MAX_PINGS_WO_DATA_THROTTLE
+inline bool IsMaxPingsWoDataThrottleEnabled() {
+  return IsExperimentEnabled(kExperimentIdMaxPingsWoDataThrottle);
+}
 #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT
 inline bool IsMonitoringExperimentEnabled() {
   return IsExperimentEnabled(kExperimentIdMonitoringExperiment);

src/core/lib/experiments/experiments.yaml

@@ -120,6 +120,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

@@ -79,6 +79,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,8 @@
 
 #include "gtest/gtest.h"
 
+#include "src/core/lib/experiments/experiments.h"
+
 namespace grpc_core {
 namespace {
 
@@ -47,6 +49,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 +65,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