Add the missing reset for ping clocks to avoid mistakenly sending GOAWAY

frames due to 'too_many_pings'

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

@@ -1105,7 +1105,6 @@ void grpc_chttp2_add_incoming_goaway(grpc_chttp2_transport* t,
             : static_cast<grpc_millis>(current_keepalive_time_ms *
                                        KEEPALIVE_TIME_BACKOFF_MULTIPLIER);
   }
-
   /* lie: use transient failure from the transport to indicate goaway has been
    * received */
   connectivity_state_set(t, GRPC_CHANNEL_TRANSIENT_FAILURE, "got_goaway");
@@ -1794,6 +1793,15 @@ void grpc_chttp2_add_ping_strike(grpc_chttp2_transport* t) {
   }
 }
 
+void grpc_chttp2_reset_ping_clock(grpc_chttp2_transport* t) {
+  if (!t->is_client) {
+    t->ping_recv_state.last_ping_recv_time = GRPC_MILLIS_INF_PAST;
+    t->ping_recv_state.ping_strikes = 0;
+  }
+  t->ping_state.pings_before_data_required =
+      t->ping_policy.max_pings_without_data;
+}
+
 static void perform_transport_op_locked(void* stream_op,
                                         grpc_error* /*error_ignored*/) {
   grpc_transport_op* op = static_cast<grpc_transport_op*>(stream_op);
@@ -2390,6 +2398,7 @@ static void close_from_api(grpc_chttp2_transport* t, grpc_chttp2_stream* s,
   grpc_slice_buffer_add(&t->qbuf, status_hdr);
   grpc_slice_buffer_add(&t->qbuf, message_pfx);
   grpc_slice_buffer_add(&t->qbuf, grpc_slice_ref_internal(slice));
+  grpc_chttp2_reset_ping_clock(t);
   grpc_chttp2_add_rst_stream_to_next_write(t, s->id, GRPC_HTTP2_NO_ERROR,
                                            &s->stats.outgoing);
 

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

@@ -843,6 +843,12 @@ void grpc_chttp2_ack_ping(grpc_chttp2_transport* t, uint64_t id);
     "too_many_pings" followed by immediately closing the connection. */
 void grpc_chttp2_add_ping_strike(grpc_chttp2_transport* t);
 
+/** Resets ping clock. Should be called when flushing window updates,
+ * initial/trailing metadata or data frames. For a server, it resets the number
+ * of ping strikes and the last_ping_recv_time. For a ping sender, it resets
+ * pings_before_data_required. */
+void grpc_chttp2_reset_ping_clock(grpc_chttp2_transport* t);
+
 /** add a ref to the stream and add it to the writable list;
     ref will be dropped in writing.c */
 void grpc_chttp2_mark_stream_writable(grpc_chttp2_transport* t,

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

@@ -242,7 +242,7 @@ class WriteContext {
       grpc_slice_buffer_add(
           &t_->outbuf, grpc_chttp2_window_update_create(0, transport_announce,
                                                         &throwaway_stats));
-      ResetPingClock();
+      grpc_chttp2_reset_ping_clock(t_);
     }
   }
 
@@ -287,15 +287,6 @@ class WriteContext {
     return s;
   }
 
-  void ResetPingClock() {
-    if (!t_->is_client) {
-      t_->ping_recv_state.last_ping_recv_time = GRPC_MILLIS_INF_PAST;
-      t_->ping_recv_state.ping_strikes = 0;
-    }
-    t_->ping_state.pings_before_data_required =
-        t_->ping_policy.max_pings_without_data;
-  }
-
   void IncInitialMetadataWrites() { ++initial_metadata_writes_; }
   void IncWindowUpdateWrites() { ++flow_control_writes_; }
   void IncMessageWrites() { ++message_writes_; }
@@ -474,7 +465,7 @@ class StreamWriteContext {
       };
       grpc_chttp2_encode_header(&t_->hpack_compressor, nullptr, 0,
                                 s_->send_initial_metadata, &hopt, &t_->outbuf);
-      write_context_->ResetPingClock();
+      grpc_chttp2_reset_ping_clock(t_);
       write_context_->IncInitialMetadataWrites();
     }
 
@@ -501,7 +492,7 @@ class StreamWriteContext {
     grpc_slice_buffer_add(
         &t_->outbuf, grpc_chttp2_window_update_create(s_->id, stream_announce,
                                                       &s_->stats.outgoing));
-    write_context_->ResetPingClock();
+    grpc_chttp2_reset_ping_clock(t_);
     write_context_->IncWindowUpdateWrites();
   }
 
@@ -543,7 +534,7 @@ class StreamWriteContext {
         }
       }
     }
-    write_context_->ResetPingClock();
+    grpc_chttp2_reset_ping_clock(t_);
     if (data_send_context.is_last_frame()) {
       SentLastFrame();
     }
@@ -584,7 +575,7 @@ class StreamWriteContext {
                                 s_->send_trailing_metadata, &hopt, &t_->outbuf);
     }
     write_context_->IncTrailingMetadataWrites();
-    write_context_->ResetPingClock();
+    grpc_chttp2_reset_ping_clock(t_);
     SentLastFrame();
 
     write_context_->NoteScheduledResults();

test/core/transport/chttp2/BUILD

@@ -143,6 +143,21 @@ grpc_cc_test(
     ],
 )
 
+grpc_cc_test(
+    "too_many_pings_test",
+    srcs = ["too_many_pings_test.cc"],
+    external_deps = ["gtest"],
+    deps = [
+        "//:gpr",
+        "//:grpc",
+        "//:grpc++",
+        "//test/core/end2end:cq_verifier",
+        "//test/core/util:grpc_test_util",
+        "//test/cpp/util:test_config",
+        "//test/cpp/util:test_util",
+    ],
+)
+
 grpc_cc_test(
     name = "varint_test",
     srcs = ["varint_test.cc"],

test/core/transport/chttp2/too_many_pings_test.cc

@@ -0,0 +1,207 @@
+/*
+ *
+ * Copyright 2020 gRPC authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+#include <grpc/support/port_platform.h>
+
+#include <gmock/gmock.h>
+#include <stdlib.h>
+#include <string.h>
+#include <functional>
+#include <set>
+#include <thread>
+
+#include <grpc/grpc.h>
+#include <grpc/grpc_security.h>
+#include <grpc/impl/codegen/grpc_types.h>
+#include <grpc/slice.h>
+#include <grpc/support/alloc.h>
+#include <grpc/support/log.h>
+#include <grpc/support/string_util.h>
+#include <grpc/support/time.h>
+
+#include <grpcpp/impl/codegen/service_type.h>
+#include <grpcpp/server_builder.h>
+
+#include "src/core/lib/gpr/useful.h"
+#include "src/core/lib/gprpp/host_port.h"
+#include "src/core/lib/gprpp/thd.h"
+#include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/security/credentials/alts/alts_credentials.h"
+#include "src/core/lib/security/credentials/credentials.h"
+#include "src/core/lib/security/security_connector/alts/alts_security_connector.h"
+#include "src/core/lib/slice/slice_string_helpers.h"
+
+#include "test/core/util/memory_counters.h"
+#include "test/core/util/port.h"
+#include "test/core/util/test_config.h"
+
+#include "test/core/end2end/cq_verifier.h"
+
+namespace {
+
+void* tag(int i) { return (void*)static_cast<intptr_t>(i); }
+
+// Perform a simple RPC and capture the value of the authority header
+// metadata sent to the server, as a string.
+grpc_status_code PerformCall(grpc_channel* channel, grpc_server* server,
+                             grpc_completion_queue* cq) {
+  grpc_call* c;
+  grpc_call* s;
+  cq_verifier* cqv = cq_verifier_create(cq);
+  grpc_op ops[6];
+  grpc_op* op;
+  grpc_metadata_array initial_metadata_recv;
+  grpc_metadata_array trailing_metadata_recv;
+  grpc_metadata_array request_metadata_recv;
+  grpc_call_details call_details;
+  grpc_status_code status;
+  grpc_call_error error;
+  grpc_slice details;
+  gpr_timespec deadline = grpc_timeout_seconds_to_deadline(5);
+  // Start a call
+  c = grpc_channel_create_call(channel, nullptr, GRPC_PROPAGATE_DEFAULTS, cq,
+                               grpc_slice_from_static_string("/foo"), nullptr,
+                               deadline, nullptr);
+  GPR_ASSERT(c);
+  grpc_metadata_array_init(&initial_metadata_recv);
+  grpc_metadata_array_init(&trailing_metadata_recv);
+  grpc_metadata_array_init(&request_metadata_recv);
+  grpc_call_details_init(&call_details);
+  memset(ops, 0, sizeof(ops));
+  op = ops;
+  op->op = GRPC_OP_SEND_INITIAL_METADATA;
+  op->data.send_initial_metadata.count = 0;
+  op->flags = 0;
+  op->reserved = nullptr;
+  op++;
+  op->op = GRPC_OP_RECV_STATUS_ON_CLIENT;
+  op->data.recv_status_on_client.trailing_metadata = &trailing_metadata_recv;
+  op->data.recv_status_on_client.status = &status;
+  op->data.recv_status_on_client.status_details = &details;
+  op->flags = 0;
+  op->reserved = nullptr;
+  op++;
+  error = grpc_call_start_batch(c, ops, static_cast<size_t>(op - ops), tag(1),
+                                nullptr);
+  GPR_ASSERT(GRPC_CALL_OK == error);
+  // Request a call on the server
+  error = grpc_server_request_call(server, &s, &call_details,
+                                   &request_metadata_recv, cq, cq, tag(101));
+  GPR_ASSERT(GRPC_CALL_OK == error);
+  CQ_EXPECT_COMPLETION(cqv, tag(101), 1);
+  cq_verify(cqv);
+  grpc_call_cancel_with_status(s, GRPC_STATUS_PERMISSION_DENIED, "test status",
+                               nullptr);
+  CQ_EXPECT_COMPLETION(cqv, tag(1), 1);
+  cq_verify(cqv);
+  // cleanup
+  grpc_slice_unref(details);
+  grpc_metadata_array_destroy(&initial_metadata_recv);
+  grpc_metadata_array_destroy(&trailing_metadata_recv);
+  grpc_metadata_array_destroy(&request_metadata_recv);
+  grpc_call_details_destroy(&call_details);
+  grpc_call_unref(c);
+  grpc_call_unref(s);
+  cq_verifier_destroy(cqv);
+  return status;
+}
+
+// Perform a few ALTS handshakes sequentially (using the fake, in-process ALTS
+// handshake server).
+TEST(TooManyPings, TestLotsOfHeaderOnlyRpcsDoesntGiveTooManyPings) {
+  grpc_completion_queue* cq = grpc_completion_queue_create_for_next(nullptr);
+  // create the server
+  grpc_server* server;
+  {
+    grpc_arg server_keepalive_args[] = {
+        {.type = GRPC_ARG_INTEGER,
+         .key = const_cast<char*>(GRPC_ARG_KEEPALIVE_TIME_MS),
+         .value = {.integer = 600000}},  // 10 minutes
+        {.type = GRPC_ARG_INTEGER,
+         .key = const_cast<char*>(GRPC_ARG_KEEPALIVE_TIMEOUT_MS),
+         .value = {.integer = 20000}},  // 20 seconds
+        {.type = GRPC_ARG_INTEGER,
+         .key = const_cast<char*>(
+             GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS),
+         .value = {.integer = 10000}},  // 10 seconds
+        {.type = GRPC_ARG_INTEGER,
+         .key = const_cast<char*>(GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS),
+         .value = {.integer = 1}}};
+    grpc_channel_args server_keepalive_channel_args = {
+        .num_args = GPR_ARRAY_SIZE(server_keepalive_args),
+        .args = server_keepalive_args};
+    server = grpc_server_create(&server_keepalive_channel_args, nullptr);
+  }
+  std::string server_address =
+      grpc_core::JoinHostPort("localhost", grpc_pick_unused_port_or_die());
+  grpc_server_register_completion_queue(server, cq, nullptr);
+  GPR_ASSERT(
+      grpc_server_add_insecure_http2_port(server, server_address.c_str()));
+  grpc_server_start(server);
+  // create the channel
+  grpc_channel* channel = grpc_insecure_channel_create(
+      server_address.c_str(), nullptr /* channel args */, nullptr);
+  std::map<grpc_status_code, int> statuses_and_counts;
+  const int kNumTotalRpcs = 1e5;
+  // perform an RPC
+  gpr_log(GPR_INFO,
+          "Performing %d total RPCs and expecting them all to receive status "
+          "PERMISSION_DENIED (%d)",
+          kNumTotalRpcs, GRPC_STATUS_PERMISSION_DENIED);
+  for (int i = 0; i < kNumTotalRpcs; i++) {
+    grpc_status_code status = PerformCall(channel, server, cq);
+    statuses_and_counts[status] += 1;
+  }
+  int num_not_cancelled = 0;
+  for (auto itr = statuses_and_counts.begin(); itr != statuses_and_counts.end();
+       itr++) {
+    if (itr->first != GRPC_STATUS_PERMISSION_DENIED) {
+      num_not_cancelled += itr->second;
+    }
+    gpr_log(GPR_INFO, "%d / %d RPCs received status code: %d", itr->second,
+            kNumTotalRpcs, itr->first);
+  }
+  if (num_not_cancelled > 0) {
+    gpr_log(GPR_ERROR,
+            "Expected all RPCs to receive status PERMISSION_DENIED (%d) but %d "
+            "received other status codes",
+            GRPC_STATUS_PERMISSION_DENIED, num_not_cancelled);
+    FAIL();
+  }
+  // shutdown and destroy the client and server
+  grpc_channel_destroy(channel);
+  grpc_server_shutdown_and_notify(server, cq, nullptr);
+  grpc_completion_queue_shutdown(cq);
+  while (grpc_completion_queue_next(cq, gpr_inf_future(GPR_CLOCK_REALTIME),
+                                    nullptr)
+             .type != GRPC_QUEUE_SHUTDOWN)
+    ;
+  grpc_server_destroy(server);
+  grpc_completion_queue_destroy(cq);
+}
+
+}  // namespace
+
+int main(int argc, char** argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  grpc::testing::TestEnvironment env(argc, argv);
+  grpc_init();
+  auto result = RUN_ALL_TESTS();
+  grpc_shutdown();
+  return result;
+}