[PH2] Misc items

1. Move `SourceConstructed` to after the party is instantiated.
2. Update TODOs and comments.
3. Add debug info where mark (@roth) had left a TODO.
4. Rename GetActiveStreamCount to GetActiveStreamCountLocked

PiperOrigin-RevId: 839746883

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 0f553b8eb36a5b8cedcd34f545b4348f876e75d1..4722049a39a0cd08f0bcf80135d63cd834ef1464 100644 (file)
--- a/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
+++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
@@ -736,7 +736,7 @@ auto Http2ClientTransport::ProcessOneFrame(Http2Frame frame) {
         return self->ProcessHttp2SecurityFrame(std::move(frame));
       },
       [](GRPC_UNUSED Http2UnknownFrame frame) {
-        // As per HTTP2 RFC, implementations MUST ignore and discard frames of
+        // RFC9113: Implementations MUST ignore and discard frames of
         // unknown types.
         return Http2Status::Ok();
       },
@@ -903,7 +903,7 @@ auto Http2ClientTransport::FlowControlPeriodicUpdateLoop() {
                 // done. We must continue to do PeriodicUpdate once BDP is in
                 // place.
                 MutexLock lock(&self->transport_mutex_);
-                if (self->GetActiveStreamCount() == 0) {
+                if (self->GetActiveStreamCountLocked() == 0) {
                   self->AddPeriodicUpdatePromiseWaker();
                   return Pending{};
                 }
@@ -1275,7 +1275,7 @@ void Http2ClientTransport::AddToStreamList(RefCountedPtr<Stream> stream) {
         << stream->GetStreamId();
     stream_list_.emplace(stream->GetStreamId(), stream);
     // TODO(tjagtap) [PH2][P2][BDP] Remove this when the BDP code is done.
-    if (GetActiveStreamCount() == 1) {
+    if (GetActiveStreamCountLocked() == 1) {
       should_wake_periodic_updates = true;
     }
   }
@@ -1380,8 +1380,6 @@ Http2ClientTransport::Http2ClientTransport(
       ztrace_collector_(std::make_shared<PromiseHttp2ZTraceCollector>()),
       should_stall_read_loop_(false) {
   GRPC_HTTP2_CLIENT_DLOG << "Http2ClientTransport Constructor Begin";
-  SourceConstructed();
-
   // Initialize the general party and write party.
   auto general_party_arena = SimpleArenaAllocator(0)->MakeArena();
   general_party_arena->SetContext<EventEngine>(event_engine_.get());
@@ -1410,6 +1408,7 @@ Http2ClientTransport::Http2ClientTransport(
 
   GRPC_DCHECK(ping_manager_.has_value());
   GRPC_DCHECK(keepalive_manager_.has_value());
+  SourceConstructed();
   GRPC_HTTP2_CLIENT_DLOG << "Http2ClientTransport Constructor End";
 }
 
@@ -1685,9 +1684,9 @@ void Http2ClientTransport::MaybeSpawnCloseTransport(Http2Status http2_status,
   absl::flat_hash_map<uint32_t, RefCountedPtr<Stream>> stream_list =
       std::move(stream_list_);
   stream_list_.clear();
-  // TODO(tjagtap) : [PH2][P2] : Provide better disconnect info here.
-  ReportDisconnectionLocked(http2_status.GetAbslConnectionError(), {},
-                            "transport closed");
+  ReportDisconnectionLocked(
+      http2_status.GetAbslConnectionError(), {},
+      absl::StrCat("Transport closed: ", http2_status.DebugString()).c_str());
   lock.Release();
 
   SpawnInfallibleTransportParty(
@@ -1741,12 +1740,12 @@ bool Http2ClientTransport::CanCloseTransportLocked() const {
   // max allowed stream id, then no more streams can be created and it is
   // safe to close the transport.
   GRPC_HTTP2_CLIENT_DLOG << "Http2ClientTransport::CanCloseTransportLocked "
-                            "GetActiveStreamCount="
-                         << GetActiveStreamCount()
+                            "GetActiveStreamCountLocked="
+                         << GetActiveStreamCountLocked()
                          << " PeekNextStreamId=" << PeekNextStreamId()
                          << " GetMaxAllowedStreamId="
                          << GetMaxAllowedStreamId();
-  return GetActiveStreamCount() == 0 &&
+  return GetActiveStreamCountLocked() == 0 &&
          PeekNextStreamId() > GetMaxAllowedStreamId();
 }
 

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 457b95f1a08d66d5fbf400ed41e516962ccdb672..0179b305c97057641e1fa33434b098f498220153 100644 (file)
--- a/src/core/ext/transport/chttp2/transport/http2_client_transport.h
+++ b/src/core/ext/transport/chttp2/transport/http2_client_transport.h
@@ -308,7 +308,7 @@ class Http2ClientTransport final : public ClientTransport,
   // 4. Application abort: In this case, multiplexer loop will write RST stream
   //    frame to the endpoint and close the stream from reads and writes. This
   //    then follows the same reasoning as case 1.
-  inline uint32_t GetActiveStreamCount() const
+  inline uint32_t GetActiveStreamCountLocked() const
       ABSL_EXCLUSIVE_LOCKS_REQUIRED(transport_mutex_) {
     return stream_list_.size();
   }
@@ -333,7 +333,7 @@ class Http2ClientTransport final : public ClientTransport,
     // implemented.
     {
       MutexLock lock(&transport_mutex_);
-      if (GetActiveStreamCount() >=
+      if (GetActiveStreamCountLocked() >=
           settings_->peer().max_concurrent_streams()) {
         return absl::ResourceExhaustedError("Reached max concurrent streams");
       }
@@ -559,7 +559,8 @@ class Http2ClientTransport final : public ClientTransport,
   // Ping Helper functions
   Duration NextAllowedPingInterval() {
     MutexLock lock(&transport_mutex_);
-    return (!keepalive_permit_without_calls_ && GetActiveStreamCount() == 0)
+    return (!keepalive_permit_without_calls_ &&
+            GetActiveStreamCountLocked() == 0)
                ? Duration::Hours(2)
                : Duration::Seconds(1);
   }
@@ -661,7 +662,7 @@ class Http2ClientTransport final : public ClientTransport,
       {
         MutexLock lock(&transport_->transport_mutex_);
         need_to_send_ping = (transport_->keepalive_permit_without_calls_ ||
-                             transport_->GetActiveStreamCount() > 0);
+                             transport_->GetActiveStreamCountLocked() > 0);
       }
       return need_to_send_ping;
     }