HTTP: Send GOAWAY frame with last stream ID on immediate disconnect (#29462)

* Send GOAWAYs even for immediate disconnects

* Add test

* Fix tests

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

@@ -1859,15 +1859,22 @@ class GracefulGoaway : public grpc_core::RefCounted<GracefulGoaway> {
 
 }  // namespace
 
-static void send_goaway(grpc_chttp2_transport* t, grpc_error_handle error) {
+static void send_goaway(grpc_chttp2_transport* t, grpc_error_handle error,
+                        bool immediate_disconnect_hint) {
   grpc_http2_error_code http_error;
   std::string message;
   grpc_error_get_status(error, grpc_core::Timestamp::InfFuture(), nullptr,
                         &message, &http_error, nullptr);
-  if (!t->is_client && http_error == GRPC_HTTP2_NO_ERROR) {
+  if (!t->is_client && http_error == GRPC_HTTP2_NO_ERROR &&
+      !immediate_disconnect_hint) {
     // Do a graceful shutdown.
+    if (t->sent_goaway_state == GRPC_CHTTP2_NO_GOAWAY_SEND) {
       GracefulGoaway::Start(t);
     } else {
+      // Graceful GOAWAY is already in progress.
+    }
+  } else if (t->sent_goaway_state == GRPC_CHTTP2_NO_GOAWAY_SEND ||
+             t->sent_goaway_state == GRPC_CHTTP2_GRACEFUL_GOAWAY) {
     // We want to log this irrespective of whether http tracing is enabled
     gpr_log(GPR_DEBUG, "%s: Sending goaway err=%s", t->peer_string.c_str(),
             grpc_error_std_string(error).c_str());
@@ -1875,6 +1882,8 @@ static void send_goaway(grpc_chttp2_transport* t, grpc_error_handle error) {
     grpc_chttp2_goaway_append(
         t->last_new_stream_id, static_cast<uint32_t>(http_error),
         grpc_slice_from_cpp_string(std::move(message)), &t->qbuf);
+  } else {
+    // Final GOAWAY has already been sent.
   }
   grpc_chttp2_initiate_write(t, GRPC_CHTTP2_INITIATE_WRITE_GOAWAY_SENT);
   GRPC_ERROR_UNREF(error);
@@ -1886,7 +1895,8 @@ void grpc_chttp2_add_ping_strike(grpc_chttp2_transport* t) {
     send_goaway(t,
                 grpc_error_set_int(
                     GRPC_ERROR_CREATE_FROM_STATIC_STRING("too_many_pings"),
-                    GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_ENHANCE_YOUR_CALM));
+                    GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_ENHANCE_YOUR_CALM),
+                /*immediate_disconnect_hint=*/true);
     // The transport will be closed after the write is done
     close_transport_locked(
         t, grpc_error_set_int(
@@ -1911,7 +1921,7 @@ static void perform_transport_op_locked(void* stream_op,
       static_cast<grpc_chttp2_transport*>(op->handler_private.extra_arg);
 
   if (op->goaway_error != GRPC_ERROR_NONE) {
-    send_goaway(t, op->goaway_error);
+    send_goaway(t, op->goaway_error, /*immediate_disconnect_hint=*/false);
   }
 
   if (op->set_accept_stream) {
@@ -1941,6 +1951,8 @@ static void perform_transport_op_locked(void* stream_op,
   }
 
   if (op->disconnect_with_error != GRPC_ERROR_NONE) {
+    send_goaway(t, GRPC_ERROR_REF(op->disconnect_with_error),
+                /*immediate_disconnect_hint=*/true);
     close_transport_locked(t, op->disconnect_with_error);
   }
 
@@ -3224,7 +3236,8 @@ static void benign_reclaimer_locked(void* arg, grpc_error_handle error) {
     send_goaway(t,
                 grpc_error_set_int(
                     GRPC_ERROR_CREATE_FROM_STATIC_STRING("Buffers full"),
-                    GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_ENHANCE_YOUR_CALM));
+                    GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_ENHANCE_YOUR_CALM),
+                /*immediate_disconnect_hint=*/true);
   } else if (error == GRPC_ERROR_NONE &&
              GRPC_TRACE_FLAG_ENABLED(grpc_resource_quota_trace)) {
     gpr_log(GPR_INFO,

test/core/transport/chttp2/graceful_shutdown_test.cc

@@ -170,10 +170,11 @@ class GracefulShutdownTest : public ::testing::Test {
     done = true;
   }
 
-  void WaitForGoaway(uint32_t last_stream_id) {
+  void WaitForGoaway(uint32_t last_stream_id, uint32_t error_code = 0,
+                     grpc_slice slice = grpc_empty_slice()) {
     grpc_slice_buffer buffer;
     grpc_slice_buffer_init(&buffer);
-    grpc_chttp2_goaway_append(last_stream_id, 0, grpc_empty_slice(), &buffer);
+    grpc_chttp2_goaway_append(last_stream_id, error_code, slice, &buffer);
     std::string expected_bytes;
     for (size_t i = 0; i < buffer.count; ++i) {
       absl::StrAppend(&expected_bytes, StringViewFromSlice(buffer.slices[i]));
@@ -411,6 +412,21 @@ TEST_F(GracefulShutdownTest, UnresponsiveClient) {
   cq_verify(cqv_);
 }
 
+// Test that servers send a GOAWAY with the last stream ID even when the
+// transport is disconnected without letting Graceful GOAWAY complete
+// successfully.
+TEST_F(GracefulShutdownTest, GoawayReceivedOnServerDisconnect) {
+  // Initiate shutdown on the server and immediately disconnect.
+  grpc_server_shutdown_and_notify(server_, cq_, Tag(1));
+  grpc_server_cancel_all_calls(server_);
+  // Wait for final goaway.
+  WaitForGoaway(/*last_stream_id=*/0, /*error_code=*/2,
+                grpc_slice_from_static_string("Cancelling all calls"));
+  // The shutdown should successfully complete.
+  CQ_EXPECT_COMPLETION(cqv_, Tag(1), true);
+  cq_verify(cqv_);
+}
+
 }  // namespace
 }  // namespace grpc_core