diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index 053677378a6..7b41b29566a 100644 --- a/bazel/experiments.bzl +++ b/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", diff --git a/include/grpc/impl/channel_arg_names.h b/include/grpc/impl/channel_arg_names.h index 663e9620a34..efd380b8681 100644 --- 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 index fed8ca20371..143b08a67c7 100644 --- 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 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(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_, - ", 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 index b9501686d95..6dab7a361bc 100644 --- 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 index f54990cac18..b4ba5bdba89 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/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, diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index 999ee24a8ed..50dc88d8269 100644 --- a/src/core/lib/experiments/experiments.h +++ b/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); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index 4d1a27e4d3c..f26fc02f27e 100644 --- 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 index 65d35a1bd4e..fe679a5718e 100644 --- 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 index 1aa247e46e3..51a799549b2 100644 --- 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 index be443468bc9..46ddd0501fd 100644 --- 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(result)); + EXPECT_EQ(absl::get(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(result)); + EXPECT_EQ(absl::get(result).wait, + Duration::Minutes(1)); +} + TEST(PingRatePolicy, RateThrottlingWorks) { Chttp2PingRatePolicy policy{ChannelArgs(), false}; // Observe that we can fail if we send in a tight loop