From a1620ce2bd8613a5610deb6d361125df438bf401 Mon Sep 17 00:00:00 2001
From: Yash Tibrewal <yashkt@google.com>
Date: Wed, 9 Mar 2022 17:12:03 -0800
Subject: [PATCH] HTTP2: Don't throttle pings from the server (#29053)

* HTTP2: Don't throttle pings from the server

* Reviewer comments
---
 include/grpc/impl/codegen/grpc_types.h        |  6 ++--
 .../ext/transport/chttp2/transport/writing.cc | 36 ++++++++++++-------
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h
index 3fa90346b4b..5ed6bd55a0c 100644
--- a/include/grpc/impl/codegen/grpc_types.h
+++ b/include/grpc/impl/codegen/grpc_types.h
@@ -219,7 +219,7 @@ typedef struct {
   "grpc.http2.min_ping_interval_without_data_ms"
 /** Channel arg to override the http2 :scheme header */
 #define GRPC_ARG_HTTP2_SCHEME "grpc.http2_scheme"
-/** How many pings can we send before needing to send a
+/** 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) */
 #define GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA \
@@ -241,8 +241,8 @@ typedef struct {
     not receive the ping ack, it will close the transport. Int valued,
     milliseconds. */
 #define GRPC_ARG_KEEPALIVE_TIMEOUT_MS "grpc.keepalive_timeout_ms"
-/** Is it permissible to send keepalive pings without any outstanding streams.
-    Int valued, 0(false)/1(true). */
+/** Is it permissible to send keepalive pings from the client without any
+   outstanding streams. Int valued, 0(false)/1(true). */
 #define GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS \
   "grpc.keepalive_permit_without_calls"
 /** Default authority to pass if none specified on call construction. A string.
diff --git a/src/core/ext/transport/chttp2/transport/writing.cc b/src/core/ext/transport/chttp2/transport/writing.cc
index 41c7d0f7fd2..0c7b6760d6d 100644
--- a/src/core/ext/transport/chttp2/transport/writing.cc
+++ b/src/core/ext/transport/chttp2/transport/writing.cc
@@ -60,15 +60,15 @@ static void maybe_initiate_ping(grpc_chttp2_transport* t) {
     }
     return;
   }
-  if (t->ping_state.pings_before_data_required == 0 &&
+  if (t->is_client && t->ping_state.pings_before_data_required == 0 &&
       t->ping_policy.max_pings_without_data != 0) {
     /* need to receive something of substance before sending a ping again */
     if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace) ||
         GRPC_TRACE_FLAG_ENABLED(grpc_bdp_estimator_trace) ||
         GRPC_TRACE_FLAG_ENABLED(grpc_keepalive_trace)) {
-      gpr_log(GPR_INFO, "%s: Ping delayed [%s]: too many recent pings: %d/%d",
-              t->is_client ? "CLIENT" : "SERVER", t->peer_string.c_str(),
-              t->ping_state.pings_before_data_required,
+      gpr_log(GPR_INFO,
+              "CLIENT: Ping delayed [%s]: too many recent pings: %d/%d",
+              t->peer_string.c_str(), t->ping_state.pings_before_data_required,
               t->ping_policy.max_pings_without_data);
     }
     return;
@@ -79,13 +79,22 @@ static void maybe_initiate_ping(grpc_chttp2_transport* t) {
   grpc_core::ExecCtx::Get()->InvalidateNow();
   grpc_core::Timestamp now = grpc_core::ExecCtx::Get()->Now();
 
-  grpc_core::Duration next_allowed_ping_interval =
-      (t->keepalive_permit_without_calls == 0 &&
-       grpc_chttp2_stream_map_size(&t->stream_map) == 0)
-          ? grpc_core::Duration::Hours(2)
-          : grpc_core::Duration::Seconds(
-                1); /* A second is added to deal with network delays and timing
-                       imprecision */
+  grpc_core::Duration next_allowed_ping_interval = grpc_core::Duration::Zero();
+  if (t->is_client) {
+    next_allowed_ping_interval =
+        (t->keepalive_permit_without_calls == 0 &&
+         grpc_chttp2_stream_map_size(&t->stream_map) == 0)
+            ? grpc_core::Duration::Hours(2)
+            : grpc_core::Duration::Seconds(1); /* A second is added to deal with
+                         network delays and timing imprecision */
+  } else {
+    // The gRPC keepalive spec doesn't call for any throttling on the server
+    // side, but we are adding some throttling for protection anyway.
+    next_allowed_ping_interval =
+        t->keepalive_time == grpc_core::Duration::Infinity()
+            ? grpc_core::Duration::Seconds(20)
+            : t->keepalive_time / 2;
+  }
   grpc_core::Timestamp next_allowed_ping =
       t->ping_state.last_ping_sent_time + next_allowed_ping_interval;
 
@@ -96,7 +105,8 @@ static void maybe_initiate_ping(grpc_chttp2_transport* t) {
         GRPC_TRACE_FLAG_ENABLED(grpc_keepalive_trace)) {
       gpr_log(
           GPR_INFO,
-          "%s: Ping delayed [%s]: not enough time elapsed since last ping. "
+          "%s: Ping delayed [%s]: not enough time elapsed since last "
+          "ping. "
           " Last ping %" PRId64 ": Next ping %" PRId64 ": Now %" PRId64,
           t->is_client ? "CLIENT" : "SERVER", t->peer_string.c_str(),
           t->ping_state.last_ping_sent_time.milliseconds_after_process_epoch(),
@@ -114,6 +124,7 @@ static void maybe_initiate_ping(grpc_chttp2_transport* t) {
     }
     return;
   }
+  t->ping_state.last_ping_sent_time = now;
 
   pq->inflight_id = t->ping_ctr;
   t->ping_ctr++;
@@ -124,7 +135,6 @@ static void maybe_initiate_ping(grpc_chttp2_transport* t) {
   grpc_slice_buffer_add(&t->outbuf,
                         grpc_chttp2_ping_create(false, pq->inflight_id));
   GRPC_STATS_INC_HTTP2_PINGS_SENT();
-  t->ping_state.last_ping_sent_time = now;
   if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace) ||
       GRPC_TRACE_FLAG_ENABLED(grpc_bdp_estimator_trace) ||
       GRPC_TRACE_FLAG_ENABLED(grpc_keepalive_trace)) {