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

yashykt · copybara-github · commit a88ee9ef99a9 · 2024-05-28T19:42:25.000-07:00
Implements grpc/proposal#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=#36374 from yashykt:ThrottlePings b5bd42a PiperOrigin-RevId: 638110795
diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl
diff --git a/include/grpc/impl/channel_arg_names.h b/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
-   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
diff --git a/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc b/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)
-                                .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.
@@ -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,26 +76,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_ > 0) {
+    --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());
 }
 
diff --git a/src/core/ext/transport/chttp2/transport/ping_rate_policy.h b/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();
 };
 
diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc
diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h
diff --git a/src/core/lib/experiments/experiments.yaml b/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
diff --git a/src/core/lib/experiments/rollouts.yaml b/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
diff --git a/test/core/end2end/tests/bad_ping.cc b/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()
diff --git a/test/core/transport/chttp2/ping_rate_policy_test.cc b/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
