[PH2][FlowControl][Bug] Stall ReadLoop

Prioritize sending flow control updates over reading data. If we continue reading while Urgent flow control updates are pending, we might exhaust the flow control window. This prevents us from sending window updates to the peer, causing the peer to block unnecessarily while waiting for flow control tokens.

PiperOrigin-RevId: 827363513

diff --git a/src/core/BUILD b/src/core/BUILD
index b9f2fbc522beae520e4693ebc95275d5c0096799..644618b274c0ce3c1f0c69e71599ed9aa4276430 100644 (file)
--- a/src/core/BUILD
+++ b/src/core/BUILD
@@ -8374,6 +8374,7 @@ grpc_cc_library(
         "check_class_size",
         "closure",
         "connectivity_state",
+        "context",
         "flow_control_manager",
         "for_each",
         "goaway",

diff --git a/src/core/ext/transport/chttp2/transport/http2_client_transport.cc b/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
index af4c1f5ca3eace77e5fed16e9e39e727e70fcf3d..8e514e6043193fc0cd93967b062ca6fee35db07c 100644 (file)
--- a/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
+++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
@@ -56,6 +56,8 @@
 #include "src/core/lib/channel/channel_args.h"
 #include "src/core/lib/debug/trace.h"
 #include "src/core/lib/iomgr/exec_ctx.h"
+#include "src/core/lib/promise/activity.h"
+#include "src/core/lib/promise/context.h"
 #include "src/core/lib/promise/for_each.h"
 #include "src/core/lib/promise/if.h"
 #include "src/core/lib/promise/loop.h"
@@ -728,6 +730,13 @@ auto Http2ClientTransport::ReadAndProcessOneFrame() {
               }
               return absl::OkStatus();
             }));
+      },
+      [self = RefAsSubclass<Http2ClientTransport>()]() -> Poll<absl::Status> {
+        if (self->should_stall_read_loop_) {
+          self->read_loop_waker_ = GetContext<Activity>()->MakeNonOwningWaker();
+          return Pending{};
+        }
+        return absl::OkStatus();
       }));
 }
 
@@ -806,6 +815,12 @@ void Http2ClientTransport::ActOnFlowControlAction(
       /*enable_preferred_rx_crypto_frame_advertisement=*/true);
 
   if (action.AnyUpdateImmediately()) {
+    // Prioritize sending flow control updates over reading data. If we
+    // continue reading while urgent flow control updates are pending, we might
+    // exhaust the flow control window. This prevents us from sending window
+    // updates to the peer, causing the peer to block unnecessarily while
+    // waiting for flow control tokens.
+    should_stall_read_loop_ = true;
     SpawnGuardedTransportParty("SendControlFrames", TriggerWriteCycle());
   }
 }
@@ -864,6 +879,10 @@ void Http2ClientTransport::NotifyControlFramesWriteDone() {
   // Notify Control modules that we have sent the frames.
   // All notifications are expected to be synchronous.
   GRPC_HTTP2_CLIENT_DLOG << "Http2ClientTransport NotifyControlFramesWriteDone";
+  if (should_stall_read_loop_) {
+    should_stall_read_loop_ = false;
+    read_loop_waker_.Wakeup();
+  }
   ping_manager_.NotifyPingSent(ping_timeout_);
   goaway_manager_.NotifyGoawaySent();
 }
@@ -1224,7 +1243,8 @@ Http2ClientTransport::Http2ClientTransport(
           "PH2_Client",
           channel_args.GetBool(GRPC_ARG_HTTP2_BDP_PROBE).value_or(true),
           &memory_owner_),
-      ztrace_collector_(std::make_shared<PromiseHttp2ZTraceCollector>()) {
+      ztrace_collector_(std::make_shared<PromiseHttp2ZTraceCollector>()),
+      should_stall_read_loop_(false) {
   GRPC_HTTP2_CLIENT_DLOG << "Http2ClientTransport Constructor Begin";
   SourceConstructed();
 

diff --git a/src/core/ext/transport/chttp2/transport/http2_client_transport.h b/src/core/ext/transport/chttp2/transport/http2_client_transport.h
index 3ac878e6d25143fae53e5ce34bf03bd1e6dd4d61..6f5ed4c6e90639ba40664e160a9410307673864a 100644 (file)
--- a/src/core/ext/transport/chttp2/transport/http2_client_transport.h
+++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.h
@@ -744,6 +744,12 @@ class Http2ClientTransport final : public ClientTransport,
 
   // TODO(tjagtap) [PH2][P2][BDP] Remove this when the BDP code is done.
   Waker periodic_updates_waker_;
+
+  // TODO(tjagtap) [PH2][P2][Settings] Set this to true when we receive settings
+  // that appear "Urgent". Example - initial window size 0 is urgent because it
+  // indicates extreme memory pressure on the server.
+  bool should_stall_read_loop_;
+  Waker read_loop_waker_;
 };
 
 // Since the corresponding class in CHTTP2 is about 3.9KB, our goal is to