[PSM Interop] Migrate to Artifact Registry (v1.54.x backport) (#36319)

Backport of #36273 to v1.54.x.
---
Migrate PSM Interop images from Container Registry (gcr.io) to Artifact
Registry (pkg.dev).

BUILD

@@ -183,7 +183,7 @@ g_stands_for = "gracious"  # @unused
 
 core_version = "31.0.0"  # @unused
 
-version = "1.54.0-dev"  # @unused
+version = "1.54.3"  # @unused
 
 GPR_PUBLIC_HDRS = [
     "include/grpc/support/alloc.h",
@@ -1943,7 +1943,6 @@ grpc_cc_library(
         "//src/core:channel_args",
         "//src/core:channel_init",
         "//src/core:closure",
-        "//src/core:default_event_engine",
         "//src/core:error",
         "//src/core:gpr_atm",
         "//src/core:gpr_manual_constructor",

CMakeLists.txt

@@ -25,12 +25,12 @@
 cmake_minimum_required(VERSION 3.8)
 
 set(PACKAGE_NAME          "grpc")
-set(PACKAGE_VERSION       "1.54.0-dev")
+set(PACKAGE_VERSION       "1.54.3")
 set(gRPC_CORE_VERSION     "31.0.0")
 set(gRPC_CORE_SOVERSION   "31")
-set(gRPC_CPP_VERSION      "1.54.0-dev")
+set(gRPC_CPP_VERSION      "1.54.3")
 set(gRPC_CPP_SOVERSION    "1.54")
-set(gRPC_CSHARP_VERSION   "2.54.0-dev")
+set(gRPC_CSHARP_VERSION   "2.54.3")
 set(gRPC_CSHARP_SOVERSION "2.54")
 set(PACKAGE_STRING        "${PACKAGE_NAME} ${PACKAGE_VERSION}")
 set(PACKAGE_TARNAME       "${PACKAGE_NAME}-${PACKAGE_VERSION}")
@@ -302,7 +302,7 @@ endif()
 if (gRPC_XDS_USER_AGENT_IS_CSHARP)
   # The value of the defines needs to contain quotes.
   # See https://github.com/grpc/grpc/blob/fbf32836a418cc84f58786700273b65cb9174e1d/src/core/ext/xds/xds_api.cc#L854
-  add_definitions("-DGRPC_XDS_USER_AGENT_NAME_SUFFIX=\"csharp\"" "-DGRPC_XDS_USER_AGENT_VERSION_SUFFIX=\"2.54.0-dev\"")
+  add_definitions("-DGRPC_XDS_USER_AGENT_NAME_SUFFIX=\"csharp\"" "-DGRPC_XDS_USER_AGENT_VERSION_SUFFIX=\"2.54.3\"")
 endif()
 
 if(UNIX AND NOT HAIKU)

Makefile

@@ -411,8 +411,8 @@ Q = @
 endif
 
 CORE_VERSION = 31.0.0
-CPP_VERSION = 1.54.0-dev
-CSHARP_VERSION = 2.54.0-dev
+CPP_VERSION = 1.54.3
+CSHARP_VERSION = 2.54.3
 
 CPPFLAGS_NO_ARCH += $(addprefix -I, $(INCLUDES)) $(addprefix -D, $(DEFINES))
 CPPFLAGS += $(CPPFLAGS_NO_ARCH) $(ARCH_FLAGS)

_metadata.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/_metadata.py.template`!!!
 
-__version__ = """1.54.0.dev0"""
+__version__ = """1.54.3"""

build_autogenerated.yaml

@@ -12809,7 +12809,6 @@ targets:
   build: test
   language: c++
   headers:
-  - test/core/util/scoped_env_var.h
   - test/core/xds/xds_transport_fake.h
   src:
   - src/proto/grpc/testing/xds/v3/base.proto

build_handwritten.yaml

@@ -16,7 +16,7 @@ settings:
   csharp_major_version: 2
   g_stands_for: gracious
   protobuf_version: 3.21.12
-  version: 1.54.0-dev
+  version: 1.54.3
 configs:
   asan:
     CC: clang

config.m4

@@ -1277,7 +1277,7 @@ if test "$PHP_GRPC" != "no"; then
     -D_HAS_EXCEPTIONS=0 -DNOMINMAX -DGRPC_ARES=0 \
     -DGRPC_POSIX_FORK_ALLOW_PTHREAD_ATFORK=1 \
     -DGRPC_XDS_USER_AGENT_NAME_SUFFIX='"\"PHP\""' \
-    -DGRPC_XDS_USER_AGENT_VERSION_SUFFIX='"\"1.54.0dev\""')
+    -DGRPC_XDS_USER_AGENT_VERSION_SUFFIX='"\"1.54.3\""')
 
   PHP_ADD_BUILD_DIR($ext_builddir/src/core/ext/filters/backend_metrics)
   PHP_ADD_BUILD_DIR($ext_builddir/src/core/ext/filters/census)

doc/g_stands_for.md

@@ -53,4 +53,4 @@
 - 1.51 'g' stands for ['galaxy'](https://github.com/grpc/grpc/tree/v1.51.x)
 - 1.52 'g' stands for ['gribkoff'](https://github.com/grpc/grpc/tree/v1.52.x)
 - 1.53 'g' stands for ['glockenspiel'](https://github.com/grpc/grpc/tree/v1.53.x)
-- 1.54 'g' stands for ['gracious'](https://github.com/grpc/grpc/tree/master)
+- 1.54 'g' stands for ['gracious'](https://github.com/grpc/grpc/tree/v1.54.x)

examples/cpp/gcp_observability/helloworld/greeter_client.cc

@@ -88,11 +88,11 @@ int main(int argc, char** argv) {
   // Turn on GCP Observability for the whole binary. Based on the configuration,
   // this will emit observability data (stats, tracing and logging) to GCP
   // backends. Note that this should be done before any other gRPC operation.
-  auto status = grpc::experimental::GcpObservabilityInit();
-  if (!status.ok()) {
-    std::cerr << "GcpObservabilityInit() failed: " << status.ToString()
-              << std::endl;
-    return static_cast<int>(status.code());
+  auto observability = grpc::GcpObservability::Init();
+  if (!observability.ok()) {
+    std::cerr << "GcpObservability::Init() failed: "
+              << observability.status().ToString() << std::endl;
+    return static_cast<int>(observability.status().code());
   }
   std::cout << "Initialized GCP Observability" << std::endl;
   // We indicate that the channel isn't authenticated (use of
@@ -102,15 +102,7 @@ int main(int argc, char** argv) {
   std::string user("world");
   std::string reply = greeter.SayHello(user);
   std::cout << "Greeter received: " << reply << std::endl;
-  // Flush out any pending Observability data
-  std::cout << "Closing GCP Observability" << std::endl;
-  grpc::experimental::GcpObservabilityClose();
-  std::cout << "Sleeping for 25 seconds to make sure Observability stats and "
-               "tracing are flushed. Don't shut off server either."
-            << std::endl;
-  // Currently, GcpObservabilityClose() only supports flushing logs. Stats and
-  // tracing get automatically flushed at a regular interval, so sleep for an
-  // interval to make sure that those are flushed too.
-  std::this_thread::sleep_for(std::chrono::seconds(25));
+  // 'observability' object going out of scope will flush observability data.
+  std::cout << "Closing and flushing GCP Observability data" << std::endl;
   return 0;
 }

examples/cpp/gcp_observability/helloworld/greeter_server.cc

@@ -99,24 +99,15 @@ int main(int argc, char** argv) {
   // Turn on GCP Observability for the whole binary. Based on the configuration,
   // this will emit observability data (stats, tracing and logging) to GCP
   // backends. Note that this should be done before any other gRPC operation.
-  auto status = grpc::experimental::GcpObservabilityInit();
-  if (!status.ok()) {
-    std::cerr << "GcpObservabilityInit() failed: " << status.ToString()
-              << std::endl;
-    return static_cast<int>(status.code());
+  auto observability = grpc::GcpObservability::Init();
+  if (!observability.ok()) {
+    std::cerr << "GcpObservability::Init() failed: "
+              << observability.status().ToString() << std::endl;
+    return static_cast<int>(observability.status().code());
   }
   std::cout << "Initialized GCP Observability" << std::endl;
   RunServer(absl::GetFlag(FLAGS_port));
-  // Flush out any pending Observability data
-  std::cout << "Closing GCP Observability" << std::endl;
-  grpc::experimental::GcpObservabilityClose();
-  std::cout << "Sleeping for 25 seconds to make sure Observability stats and "
-               "tracing are flushed.(Another Ctrl+C will immediately exit the "
-               "program.)"
-            << std::endl;
-  // Currently, GcpObservabilityClose() only supports flushing logs. Stats and
-  // tracing get automatically flushed at a regular interval, so sleep for an
-  // interval to make sure that those are flushed too.
-  std::this_thread::sleep_for(std::chrono::seconds(25));
+  // 'observability' object going out of scope will flush observability data.
+  std::cout << "Closing and flushing GCP Observability data" << std::endl;
   return 0;
 }

gRPC-C++.podspec

@@ -22,7 +22,7 @@
 Pod::Spec.new do |s|
   s.name     = 'gRPC-C++'
   # TODO (mxyan): use version that match gRPC version when pod is stabilized
-  version = '1.54.0-dev'
+  version = '1.54.3'
   s.version  = version
   s.summary  = 'gRPC C++ library'
   s.homepage = 'https://grpc.io'

gRPC-Core.podspec

@@ -21,7 +21,7 @@
 
 Pod::Spec.new do |s|
   s.name     = 'gRPC-Core'
-  version = '1.54.0-dev'
+  version = '1.54.3'
   s.version  = version
   s.summary  = 'Core cross-platform gRPC library, written in C'
   s.homepage = 'https://grpc.io'

gRPC-ProtoRPC.podspec

@@ -21,7 +21,7 @@
 
 Pod::Spec.new do |s|
   s.name     = 'gRPC-ProtoRPC'
-  version = '1.54.0-dev'
+  version = '1.54.3'
   s.version  = version
   s.summary  = 'RPC library for Protocol Buffers, based on gRPC'
   s.homepage = 'https://grpc.io'

gRPC-RxLibrary.podspec

@@ -21,7 +21,7 @@
 
 Pod::Spec.new do |s|
   s.name     = 'gRPC-RxLibrary'
-  version = '1.54.0-dev'
+  version = '1.54.3'
   s.version  = version
   s.summary  = 'Reactive Extensions library for iOS/OSX.'
   s.homepage = 'https://grpc.io'

gRPC.podspec

@@ -20,7 +20,7 @@
 
 Pod::Spec.new do |s|
   s.name     = 'gRPC'
-  version = '1.54.0-dev'
+  version = '1.54.3'
   s.version  = version
   s.summary  = 'gRPC client library for iOS/OSX'
   s.homepage = 'https://grpc.io'

include/grpcpp/alarm.h

@@ -23,7 +23,6 @@
 
 #include <functional>
 
-#include <grpc/event_engine/event_engine.h>
 #include <grpc/grpc.h>
 #include <grpcpp/completion_queue.h>
 #include <grpcpp/impl/completion_queue_tag.h>

include/grpcpp/ext/gcp_observability.h

@@ -17,25 +17,97 @@
 #ifndef GRPCPP_EXT_GCP_OBSERVABILITY_H
 #define GRPCPP_EXT_GCP_OBSERVABILITY_H
 
+#include <grpc/support/port_platform.h>
+
 #include "absl/status/status.h"
+#include "absl/status/statusor.h"
+
+#include <grpcpp/impl/grpc_library.h>
 
 namespace grpc {
+
+// GcpObservability objects follow the RAII idiom and help manage the lifetime
+// of gRPC Observability data exporting to GCP. `GcpObservability::Init()`
+// should be invoked instead to return an `GcpObservability` instance.
+// Observability data is flushed at regular intervals, and also when this
+// instance goes out of scope and its destructor is invoked.
+class GcpObservability {
+ public:
+  // Initialize GCP Observability for gRPC.
+  // This should be called before any other gRPC operations like creating a
+  // channel, server, credentials etc.
+  // The return value helps determine whether observability was
+  // successfully enabled or not. On success, an object of class `Observability`
+  // is returned. When this object goes out of scope, GCP Observability stats,
+  // tracing and logging data is flushed. On failure, the status message can be
+  // used to determine the cause of failure. It is up to the applications to
+  // either crash on failure, or continue without GCP observability being
+  // enabled. The status codes do not have any special meaning at present, and
+  // users should not make any assumptions based on the status code, other than
+  // a non-OK status code meaning that observability initialization failed.
+  //
+  // The expected usage is to call this at the top (or near the top) in
+  // main(), and let it go out of scope after all RPCs and activities that we
+  // want to observe are done. Please look at
+  // https://github.com/grpc/grpc/blob/master/examples/cpp/gcp_observability/helloworld/greeter_client.cc
+  // and
+  // https://github.com/grpc/grpc/blob/master/examples/cpp/gcp_observability/helloworld/greeter_server.cc
+  // for sample usage.
+  //
+  // It is possible for an initialized GcpObservability object to go out of
+  // scope while RPCs and other gRPC operations are still ongoing. In this case,
+  // GCP Observability tries to flush all observability data collected till that
+  // point.
+  //
+  // Note that this is a blocking call which properly sets up gRPC Observability
+  // to work with GCP and might take a few seconds to return.  Similarly, the
+  // destruction of a non-moved-from `Observability` object is also blocking
+  // since it flushes the observability data to GCP.
+  //
+  // As an implementation detail, this properly initializes the OpenCensus stats
+  // and tracing plugin, so applications do not need to perform any additional
+  // gRPC C++ OpenCensus setup/registration to get GCP Observability for gRPC.
+  static absl::StatusOr<GcpObservability> Init() GRPC_MUST_USE_RESULT;
+
+  GcpObservability() = default;
+  // Move constructor and Move-assignment operator.
+  // The moved-from object will no longer be valid and will not cause GCP
+  // Observability stats, tracing and logging data to flush.
+  GcpObservability(GcpObservability&& other) noexcept;
+  GcpObservability& operator=(GcpObservability&& other) noexcept;
+
+  // Delete copy and copy-assignment operator
+  GcpObservability(const GcpObservability&) = delete;
+  GcpObservability& operator=(const GcpObservability&) = delete;
+
+ private:
+  // Helper class that aids in implementing GCP Observability.
+  // Inheriting from GrpcLibrary makes sure that gRPC is initialized and remains
+  // initialized for the lifetime of GCP Observability. In the future, when gRPC
+  // initialization goes away, we might still want to keep gRPC Event Engine
+  // initialized, just in case, we need to perform some IO operations during
+  // observability close.
+  // Note that the lifetime guarantees are only one way, i.e., GcpObservability
+  // object guarantees that gRPC will not shutdown while the object is still in
+  // scope, but the other way around does not hold true. Even though that is not
+  // the expected usage, GCP Observability can shutdown before gRPC shuts down.
+  // It follows that gRPC should not hold any callbacks from GcpObservability. A
+  // change in this restriction should go through a design review.
+  class GcpObservabilityImpl : private internal::GrpcLibrary {
+   public:
+    ~GcpObservabilityImpl() override;
+  };
+  std::unique_ptr<GcpObservabilityImpl> impl_;
+};
+
 namespace experimental {
-
-// Initialize GCP Observability for gRPC.
-// This should be called before any other gRPC operations like creating a
-// channel, server, credentials etc.
-// The most common usage would call this at the top (or near the top) in main().
-// As an implementation detail, this properly initializes the OpenCensus stats
-// and tracing plugin, so applications do not need to perform any additional
-// gRPC C++ OpenCensus setup/registration to get GCP Observability for gRPC.
+// TODO(yashykt): Delete this after the 1.55 release.
+GRPC_DEPRECATED("Use grpc::GcpObservability::Init() instead.")
 absl::Status GcpObservabilityInit();
-
-// Gracefully shuts down GCP Observability.
-// Note that graceful shutdown for stats and tracing is not yet supported.
+GRPC_DEPRECATED("Use grpc::GcpObservability::Init() instead.")
 void GcpObservabilityClose();
-
 }  // namespace experimental
+
 }  // namespace grpc
 
 #endif  // GRPCPP_EXT_GCP_OBSERVABILITY_H

include/grpcpp/version_info.h

@@ -20,8 +20,8 @@
 
 #define GRPC_CPP_VERSION_MAJOR 1
 #define GRPC_CPP_VERSION_MINOR 54
-#define GRPC_CPP_VERSION_PATCH 0
-#define GRPC_CPP_VERSION_TAG "dev"
-#define GRPC_CPP_VERSION_STRING "1.54.0-dev"
+#define GRPC_CPP_VERSION_PATCH 3
+#define GRPC_CPP_VERSION_TAG ""
+#define GRPC_CPP_VERSION_STRING "1.54.3"
 
 #endif  // GRPCPP_VERSION_INFO_H

package.xml

@@ -13,16 +13,16 @@
  <date>2019-09-24</date>
  <time>16:06:07</time>
  <version>
-  <release>1.54.0dev</release>
-  <api>1.54.0dev</api>
+  <release>1.54.3</release>
+  <api>1.54.3</api>
  </version>
  <stability>
-  <release>beta</release>
-  <api>beta</api>
+  <release>stable</release>
+  <api>stable</api>
  </stability>
  <license>Apache 2.0</license>
  <notes>
-- gRPC Core 1.54.0 update
+- gRPC Core 1.54.3 update
  </notes>
  <contents>
   <dir baseinstalldir="/" name="/">

requirements.txt

@@ -1,5 +1,5 @@
 # GRPC Python setup requirements
 coverage>=4.0
-cython>=0.29.8
+cython>=0.29.8,<3.0.0rc1
 protobuf>=4.21.3,<5.0dev
 wheel>=0.29

setup.py

@@ -502,8 +502,8 @@ except ImportError:
             "other commands, but the extension files will fail to build.\n")
     elif need_cython:
         sys.stderr.write(
-            'We could not find Cython. Setup may take 10-20 minutes.\n')
-        SETUP_REQUIRES += ('cython>=0.23',)
+            "We could not find Cython. Setup may take 10-20 minutes.\n")
+        SETUP_REQUIRES += ("cython>=0.23,<3.0.0rc1",)
 
 COMMAND_CLASS = {
     'doc': commands.SphinxDocumentation,

src/core/BUILD

@@ -1908,6 +1908,7 @@ grpc_cc_library(
         "posix_event_engine_tcp_socket_utils",
         "socket_mutator",
         "status_helper",
+        "time",
         "//:event_engine_base_hdrs",
         "//:gpr",
     ],

src/core/ext/filters/client_channel/client_channel.cc

@@ -3037,6 +3037,8 @@ void ClientChannel::FilterBasedLoadBalancedCall::RecvInitialMetadataReady(
     // recv_initial_metadata_flags is not populated for clients
     self->call_attempt_tracer()->RecordReceivedInitialMetadata(
         self->recv_initial_metadata_);
+    auto* peer_string = self->recv_initial_metadata_->get_pointer(PeerString());
+    if (peer_string != nullptr) self->peer_string_ = peer_string->Ref();
   }
   Closure::Run(DEBUG_LOCATION, self->original_recv_initial_metadata_ready_,
                error);
@@ -3080,12 +3082,8 @@ void ClientChannel::FilterBasedLoadBalancedCall::RecvTrailingMetadataReady(
       }
     }
     absl::string_view peer_string;
-    if (self->recv_initial_metadata_ != nullptr) {
-      Slice* peer_string_slice =
-          self->recv_initial_metadata_->get_pointer(PeerString());
-      if (peer_string_slice != nullptr) {
-        peer_string = peer_string_slice->as_string_view();
-      }
+    if (self->peer_string_.has_value()) {
+      peer_string = self->peer_string_->as_string_view();
     }
     self->RecordCallCompletion(status, self->recv_trailing_metadata_,
                                self->transport_stream_stats_, peer_string);

src/core/ext/filters/client_channel/client_channel.h

@@ -63,6 +63,7 @@
 #include "src/core/lib/resolver/resolver.h"
 #include "src/core/lib/resource_quota/arena.h"
 #include "src/core/lib/service_config/service_config.h"
+#include "src/core/lib/slice/slice.h"
 #include "src/core/lib/surface/channel.h"
 #include "src/core/lib/transport/connectivity_state.h"
 #include "src/core/lib/transport/metadata_batch.h"
@@ -548,6 +549,7 @@ class ClientChannel::FilterBasedLoadBalancedCall
   CallCombiner* call_combiner_;
   grpc_polling_entity* pollent_;
   grpc_closure* on_call_destruction_complete_;
+  absl::optional<Slice> peer_string_;
 
   // Set when we get a cancel_stream op.
   grpc_error_handle cancel_error_;

src/core/ext/filters/client_channel/lb_policy/rls/rls.cc

@@ -116,7 +116,7 @@ constexpr absl::string_view kRls = "rls_experimental";
 const char kGrpc[] = "grpc";
 const char* kRlsRequestPath = "/grpc.lookup.v1.RouteLookupService/RouteLookup";
 const char* kFakeTargetFieldValue = "fake_target_field_value";
-const char* kRlsHeaderKey = "X-Google-RLS-Data";
+const char* kRlsHeaderKey = "x-google-rls-data";
 
 const Duration kDefaultLookupServiceTimeout = Duration::Seconds(10);
 const Duration kMaxMaxAge = Duration::Minutes(5);

src/core/ext/gcp/metadata_query.cc

@@ -46,16 +46,11 @@ namespace grpc_core {
 
 TraceFlag grpc_metadata_query_trace(false, "metadata_query");
 
-constexpr const char MetadataQuery::kZoneAttribute[] =
-    "/computeMetadata/v1/instance/zone";
-constexpr const char MetadataQuery::kClusterNameAttribute[] =
-    "/computeMetadata/v1/instance/attributes/cluster-name";
-constexpr const char MetadataQuery::kRegionAttribute[] =
-    "/computeMetadata/v1/instance/region";
-constexpr const char MetadataQuery::kInstanceIdAttribute[] =
-    "/computeMetadata/v1/instance/id";
-constexpr const char MetadataQuery::kIPv6Attribute[] =
-    "/computeMetadata/v1/instance/network-interfaces/0/ipv6s";
+constexpr const char MetadataQuery::kZoneAttribute[];
+constexpr const char MetadataQuery::kClusterNameAttribute[];
+constexpr const char MetadataQuery::kRegionAttribute[];
+constexpr const char MetadataQuery::kInstanceIdAttribute[];
+constexpr const char MetadataQuery::kIPv6Attribute[];
 
 MetadataQuery::MetadataQuery(
     std::string attribute, grpc_polling_entity* pollent,

src/core/ext/gcp/metadata_query.h

@@ -38,11 +38,16 @@ namespace grpc_core {
 // environment.
 class MetadataQuery : public InternallyRefCounted<MetadataQuery> {
  public:
-  static const char kZoneAttribute[];
-  static const char kClusterNameAttribute[];
-  static const char kRegionAttribute[];
-  static const char kInstanceIdAttribute[];
-  static const char kIPv6Attribute[];
+  static constexpr const char kZoneAttribute[] =
+      "/computeMetadata/v1/instance/zone";
+  static constexpr const char kClusterNameAttribute[] =
+      "/computeMetadata/v1/instance/attributes/cluster-name";
+  static constexpr const char kRegionAttribute[] =
+      "/computeMetadata/v1/instance/region";
+  static constexpr const char kInstanceIdAttribute[] =
+      "/computeMetadata/v1/instance/id";
+  static constexpr const char kIPv6Attribute[] =
+      "/computeMetadata/v1/instance/network-interfaces/0/ipv6s";
 
   MetadataQuery(
       std::string attribute, grpc_polling_entity* pollent,

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

@@ -149,7 +149,8 @@ static void enc_flush_some(huff_out* out) {
   }
 }
 
-static void enc_add2(huff_out* out, uint8_t a, uint8_t b) {
+static void enc_add2(huff_out* out, uint8_t a, uint8_t b, uint32_t* wire_size) {
+  *wire_size += 2;
   b64_huff_sym sa = huff_alphabet[a];
   b64_huff_sym sb = huff_alphabet[b];
   out->temp = (out->temp << (sa.length + sb.length)) |
@@ -159,7 +160,8 @@ static void enc_add2(huff_out* out, uint8_t a, uint8_t b) {
   enc_flush_some(out);
 }
 
-static void enc_add1(huff_out* out, uint8_t a) {
+static void enc_add1(huff_out* out, uint8_t a, uint32_t* wire_size) {
+  *wire_size += 1;
   b64_huff_sym sa = huff_alphabet[a];
   out->temp = (out->temp << sa.length) | sa.bits;
   out->temp_length += sa.length;
@@ -167,7 +169,7 @@ static void enc_add1(huff_out* out, uint8_t a) {
 }
 
 grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
-    const grpc_slice& input) {
+    const grpc_slice& input, uint32_t* wire_size) {
   size_t input_length = GRPC_SLICE_LENGTH(input);
   size_t input_triplets = input_length / 3;
   size_t tail_case = input_length % 3;
@@ -183,16 +185,17 @@ grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
   out.temp = 0;
   out.temp_length = 0;
   out.out = start_out;
+  *wire_size = 0;
 
   // encode full triplets
   for (i = 0; i < input_triplets; i++) {
     const uint8_t low_to_high = static_cast<uint8_t>((in[0] & 0x3) << 4);
     const uint8_t high_to_low = in[1] >> 4;
-    enc_add2(&out, in[0] >> 2, low_to_high | high_to_low);
+    enc_add2(&out, in[0] >> 2, low_to_high | high_to_low, wire_size);
 
     const uint8_t a = static_cast<uint8_t>((in[1] & 0xf) << 2);
     const uint8_t b = (in[2] >> 6);
-    enc_add2(&out, a | b, in[2] & 0x3f);
+    enc_add2(&out, a | b, in[2] & 0x3f, wire_size);
     in += 3;
   }
 
@@ -201,14 +204,15 @@ grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
     case 0:
       break;
     case 1:
-      enc_add2(&out, in[0] >> 2, static_cast<uint8_t>((in[0] & 0x3) << 4));
+      enc_add2(&out, in[0] >> 2, static_cast<uint8_t>((in[0] & 0x3) << 4),
+               wire_size);
       in += 1;
       break;
     case 2: {
       const uint8_t low_to_high = static_cast<uint8_t>((in[0] & 0x3) << 4);
       const uint8_t high_to_low = in[1] >> 4;
-      enc_add2(&out, in[0] >> 2, low_to_high | high_to_low);
-      enc_add1(&out, static_cast<uint8_t>((in[1] & 0xf) << 2));
+      enc_add2(&out, in[0] >> 2, low_to_high | high_to_low, wire_size);
+      enc_add1(&out, static_cast<uint8_t>((in[1] & 0xf) << 2), wire_size);
       in += 2;
       break;
     }

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

@@ -21,6 +21,8 @@
 
 #include <grpc/support/port_platform.h>
 
+#include <stdint.h>
+
 #include <grpc/slice.h>
 
 // base64 encode a slice. Returns a new slice, does not take ownership of the
@@ -36,7 +38,9 @@ grpc_slice grpc_chttp2_huffman_compress(const grpc_slice& input);
 // grpc_slice y = grpc_chttp2_huffman_compress(x);
 // grpc_core::CSliceUnref( x);
 // return y;
+// *wire_size is the length of the base64 encoded string prior to huffman
+// compression (as is needed for hpack table math)
 grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
-    const grpc_slice& input);
+    const grpc_slice& input, uint32_t* wire_size);
 
 #endif  // GRPC_SRC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_BIN_ENCODER_H

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

@@ -109,33 +109,56 @@ void HPackCompressor::Frame(const EncodeHeaderOptions& options,
   }
 }
 
-void HPackCompressor::Encoder::EmitIndexed(uint32_t elem_index) {
-  VarintWriter<1> w(elem_index);
-  w.Write(0x80, output_.AddTiny(w.length()));
+void HPackCompressor::SetMaxUsableSize(uint32_t max_table_size) {
+  max_usable_size_ = max_table_size;
+  SetMaxTableSize(std::min(table_.max_size(), max_table_size));
 }
 
+void HPackCompressor::SetMaxTableSize(uint32_t max_table_size) {
+  if (table_.SetMaxSize(std::min(max_usable_size_, max_table_size))) {
+    advertise_table_size_change_ = true;
+    if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace)) {
+      gpr_log(GPR_INFO, "set max table size from encoder to %d",
+              max_table_size);
+    }
+  }
+}
+
+namespace {
 struct WireValue {
   WireValue(uint8_t huffman_prefix, bool insert_null_before_wire_value,
             Slice slice)
       : data(std::move(slice)),
         huffman_prefix(huffman_prefix),
         insert_null_before_wire_value(insert_null_before_wire_value),
-        length(data.length() + (insert_null_before_wire_value ? 1 : 0)) {}
+        length(data.length() + (insert_null_before_wire_value ? 1 : 0)),
+        hpack_length(length) {}
+  WireValue(uint8_t huffman_prefix, bool insert_null_before_wire_value,
+            Slice slice, size_t hpack_length)
+      : data(std::move(slice)),
+        huffman_prefix(huffman_prefix),
+        insert_null_before_wire_value(insert_null_before_wire_value),
+        length(data.length() + (insert_null_before_wire_value ? 1 : 0)),
+        hpack_length(hpack_length + (insert_null_before_wire_value ? 1 : 0)) {}
   Slice data;
   const uint8_t huffman_prefix;
   const bool insert_null_before_wire_value;
   const size_t length;
+  const size_t hpack_length;
 };
 
-static WireValue GetWireValue(Slice value, bool true_binary_enabled,
-                              bool is_bin_hdr) {
+// Construct a wire value from a slice.
+// true_binary_enabled => use the true binary system
+// is_bin_hdr => the header is -bin suffixed
+WireValue GetWireValue(Slice value, bool true_binary_enabled, bool is_bin_hdr) {
   if (is_bin_hdr) {
     if (true_binary_enabled) {
       return WireValue(0x00, true, std::move(value));
     } else {
-      return WireValue(0x80, false,
-                       Slice(grpc_chttp2_base64_encode_and_huffman_compress(
-                           value.c_slice())));
+      uint32_t hpack_length;
+      Slice output(grpc_chttp2_base64_encode_and_huffman_compress(
+          value.c_slice(), &hpack_length));
+      return WireValue(0x80, false, std::move(output), hpack_length);
     }
   } else {
     // TODO(ctiller): opportunistically compress non-binary headers
@@ -175,6 +198,8 @@ class BinaryStringValue {
 
   Slice data() { return std::move(wire_value_.data); }
 
+  uint32_t hpack_length() { return wire_value_.hpack_length; }
+
  private:
   WireValue wire_value_;
   VarintWriter<1> len_val_;
@@ -214,19 +239,33 @@ class StringKey {
   Slice key_;
   VarintWriter<1> len_key_;
 };
+}  // namespace
 
-void HPackCompressor::Encoder::EmitLitHdrWithNonBinaryStringKeyIncIdx(
-    Slice key_slice, Slice value_slice) {
+namespace hpack_encoder_detail {
+void Encoder::EmitIndexed(uint32_t elem_index) {
+  VarintWriter<1> w(elem_index);
+  w.Write(0x80, output_.AddTiny(w.length()));
+}
+
+uint32_t Encoder::EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice key_slice,
+                                                         Slice value_slice) {
+  auto key_len = key_slice.length();
+  auto value_len = value_slice.length();
   StringKey key(std::move(key_slice));
   key.WritePrefix(0x40, output_.AddTiny(key.prefix_length()));
   output_.Append(key.key());
   NonBinaryStringValue emit(std::move(value_slice));
   emit.WritePrefix(output_.AddTiny(emit.prefix_length()));
+  // Allocate an index in the hpack table for this newly emitted entry.
+  // (we do so here because we know the length of the key and value)
+  uint32_t index = compressor_->table_.AllocateIndex(
+      key_len + value_len + hpack_constants::kEntryOverhead);
   output_.Append(emit.data());
+  return index;
 }
 
-void HPackCompressor::Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(
-    Slice key_slice, Slice value_slice) {
+void Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(Slice key_slice,
+                                                  Slice value_slice) {
   StringKey key(std::move(key_slice));
   key.WritePrefix(0x00, output_.AddTiny(key.prefix_length()));
   output_.Append(key.key());
@@ -235,18 +274,24 @@ void HPackCompressor::Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(
   output_.Append(emit.data());
 }
 
-void HPackCompressor::Encoder::EmitLitHdrWithBinaryStringKeyIncIdx(
-    Slice key_slice, Slice value_slice) {
+uint32_t Encoder::EmitLitHdrWithBinaryStringKeyIncIdx(Slice key_slice,
+                                                      Slice value_slice) {
+  auto key_len = key_slice.length();
   StringKey key(std::move(key_slice));
   key.WritePrefix(0x40, output_.AddTiny(key.prefix_length()));
   output_.Append(key.key());
   BinaryStringValue emit(std::move(value_slice), use_true_binary_metadata_);
   emit.WritePrefix(output_.AddTiny(emit.prefix_length()));
+  // Allocate an index in the hpack table for this newly emitted entry.
+  // (we do so here because we know the length of the key and value)
+  uint32_t index = compressor_->table_.AllocateIndex(
+      key_len + emit.hpack_length() + hpack_constants::kEntryOverhead);
   output_.Append(emit.data());
+  return index;
 }
 
-void HPackCompressor::Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(
-    uint32_t key_index, Slice value_slice) {
+void Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(uint32_t key_index,
+                                                  Slice value_slice) {
   BinaryStringValue emit(std::move(value_slice), use_true_binary_metadata_);
   VarintWriter<4> key(key_index);
   uint8_t* data = output_.AddTiny(key.length() + emit.prefix_length());
@@ -255,8 +300,8 @@ void HPackCompressor::Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(
   output_.Append(emit.data());
 }
 
-void HPackCompressor::Encoder::EmitLitHdrWithNonBinaryStringKeyNotIdx(
-    Slice key_slice, Slice value_slice) {
+void Encoder::EmitLitHdrWithNonBinaryStringKeyNotIdx(Slice key_slice,
+                                                     Slice value_slice) {
   StringKey key(std::move(key_slice));
   key.WritePrefix(0x00, output_.AddTiny(key.prefix_length()));
   output_.Append(key.key());
@@ -265,14 +310,14 @@ void HPackCompressor::Encoder::EmitLitHdrWithNonBinaryStringKeyNotIdx(
   output_.Append(emit.data());
 }
 
-void HPackCompressor::Encoder::AdvertiseTableSizeChange() {
+void Encoder::AdvertiseTableSizeChange() {
   VarintWriter<3> w(compressor_->table_.max_size());
   w.Write(0x20, output_.AddTiny(w.length()));
 }
 
-void HPackCompressor::SliceIndex::EmitTo(absl::string_view key,
-                                         const Slice& value, Encoder* encoder) {
-  auto& table = encoder->compressor_->table_;
+void SliceIndex::EmitTo(absl::string_view key, const Slice& value,
+                        Encoder* encoder) {
+  auto& table = encoder->hpack_table();
   using It = std::vector<ValueIndex>::iterator;
   It prev = values_.end();
   size_t transport_length =
@@ -291,8 +336,7 @@ void HPackCompressor::SliceIndex::EmitTo(absl::string_view key,
         encoder->EmitIndexed(table.DynamicIndex(it->index));
       } else {
         // Not current, emit a new literal and update the index.
-        it->index = table.AllocateIndex(transport_length);
-        encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
+        it->index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
             Slice::FromStaticString(key), value.Ref());
       }
       // Bubble this entry up if we can - ensures that the most used values end
@@ -310,13 +354,12 @@ void HPackCompressor::SliceIndex::EmitTo(absl::string_view key,
     prev = it;
   }
   // No hit, emit a new literal and add it to the index.
-  uint32_t index = table.AllocateIndex(transport_length);
-  encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice::FromStaticString(key),
-                                                  value.Ref());
+  uint32_t index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
+      Slice::FromStaticString(key), value.Ref());
   values_.emplace_back(value.Ref(), index);
 }
 
-void HPackCompressor::Encoder::Encode(const Slice& key, const Slice& value) {
+void Encoder::Encode(const Slice& key, const Slice& value) {
   if (absl::EndsWith(key.as_string_view(), "-bin")) {
     EmitLitHdrWithBinaryStringKeyNotIdx(key.Ref(), value.Ref());
   } else {
@@ -324,43 +367,14 @@ void HPackCompressor::Encoder::Encode(const Slice& key, const Slice& value) {
   }
 }
 
-void HPackCompressor::Encoder::Encode(HttpPathMetadata, const Slice& value) {
-  compressor_->path_index_.EmitTo(HttpPathMetadata::key(), value, this);
-}
-
-void HPackCompressor::Encoder::Encode(HttpAuthorityMetadata,
-                                      const Slice& value) {
-  compressor_->authority_index_.EmitTo(HttpAuthorityMetadata::key(), value,
-                                       this);
-}
-
-void HPackCompressor::Encoder::Encode(TeMetadata, TeMetadata::ValueType value) {
-  GPR_ASSERT(value == TeMetadata::ValueType::kTrailers);
-  EncodeAlwaysIndexed(
-      &compressor_->te_index_, "te", Slice::FromStaticString("trailers"),
-      2 /* te */ + 8 /* trailers */ + hpack_constants::kEntryOverhead);
-}
-
-void HPackCompressor::Encoder::Encode(ContentTypeMetadata,
-                                      ContentTypeMetadata::ValueType value) {
-  if (value != ContentTypeMetadata::ValueType::kApplicationGrpc) {
-    gpr_log(GPR_ERROR, "Not encoding bad content-type header");
-    return;
-  }
-  EncodeAlwaysIndexed(&compressor_->content_type_index_, "content-type",
-                      Slice::FromStaticString("application/grpc"),
-                      12 /* content-type */ + 16 /* application/grpc */ +
-                          hpack_constants::kEntryOverhead);
-}
-
-void HPackCompressor::Encoder::Encode(HttpSchemeMetadata,
-                                      HttpSchemeMetadata::ValueType value) {
+void Compressor<HttpSchemeMetadata, HttpSchemeCompressor>::EncodeWith(
+    HttpSchemeMetadata, HttpSchemeMetadata::ValueType value, Encoder* encoder) {
   switch (value) {
     case HttpSchemeMetadata::ValueType::kHttp:
-      EmitIndexed(6);  // :scheme: http
+      encoder->EmitIndexed(6);  // :scheme: http
       break;
     case HttpSchemeMetadata::ValueType::kHttps:
-      EmitIndexed(7);  // :scheme: https
+      encoder->EmitIndexed(7);  // :scheme: https
       break;
     case HttpSchemeMetadata::ValueType::kInvalid:
       Crash("invalid http scheme encoding");
@@ -368,22 +382,10 @@ void HPackCompressor::Encoder::Encode(HttpSchemeMetadata,
   }
 }
 
-void HPackCompressor::Encoder::Encode(GrpcTraceBinMetadata,
-                                      const Slice& slice) {
-  EncodeRepeatingSliceValue(GrpcTraceBinMetadata::key(), slice,
-                            &compressor_->grpc_trace_bin_index_,
-                            HPackEncoderTable::MaxEntrySize());
-}
-
-void HPackCompressor::Encoder::Encode(GrpcTagsBinMetadata, const Slice& slice) {
-  EncodeRepeatingSliceValue(GrpcTagsBinMetadata::key(), slice,
-                            &compressor_->grpc_tags_bin_index_,
-                            HPackEncoderTable::MaxEntrySize());
-}
-
-void HPackCompressor::Encoder::Encode(HttpStatusMetadata, uint32_t status) {
+void Compressor<HttpStatusMetadata, HttpStatusCompressor>::EncodeWith(
+    HttpStatusMetadata, uint32_t status, Encoder* encoder) {
   if (status == 200) {
-    EmitIndexed(8);  // :status: 200
+    encoder->EmitIndexed(8);  // :status: 200
     return;
   }
   uint8_t index = 0;
@@ -408,27 +410,28 @@ void HPackCompressor::Encoder::Encode(HttpStatusMetadata, uint32_t status) {
       break;
   }
   if (GPR_LIKELY(index != 0)) {
-    EmitIndexed(index);
+    encoder->EmitIndexed(index);
   } else {
-    EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice::FromStaticString(":status"),
-                                           Slice::FromInt64(status));
+    encoder->EmitLitHdrWithNonBinaryStringKeyNotIdx(
+        Slice::FromStaticString(":status"), Slice::FromInt64(status));
   }
 }
 
-void HPackCompressor::Encoder::Encode(HttpMethodMetadata,
-                                      HttpMethodMetadata::ValueType method) {
+void Compressor<HttpMethodMetadata, HttpMethodCompressor>::EncodeWith(
+    HttpMethodMetadata, HttpMethodMetadata::ValueType method,
+    Encoder* encoder) {
   switch (method) {
     case HttpMethodMetadata::ValueType::kPost:
-      EmitIndexed(3);  // :method: POST
+      encoder->EmitIndexed(3);  // :method: POST
       break;
     case HttpMethodMetadata::ValueType::kGet:
-      EmitIndexed(2);  // :method: GET
+      encoder->EmitIndexed(2);  // :method: GET
       break;
     case HttpMethodMetadata::ValueType::kPut:
       // Right now, we only emit PUT as a method for testing purposes, so it's
       // fine to not index it.
-      EmitLitHdrWithNonBinaryStringKeyNotIdx(Slice::FromStaticString(":method"),
-                                             Slice::FromStaticString("PUT"));
+      encoder->EmitLitHdrWithNonBinaryStringKeyNotIdx(
+          Slice::FromStaticString(":method"), Slice::FromStaticString("PUT"));
       break;
     case HttpMethodMetadata::ValueType::kInvalid:
       Crash("invalid http method encoding");
@@ -436,35 +439,31 @@ void HPackCompressor::Encoder::Encode(HttpMethodMetadata,
   }
 }
 
-void HPackCompressor::Encoder::EncodeAlwaysIndexed(uint32_t* index,
-                                                   absl::string_view key,
-                                                   Slice value,
-                                                   size_t transport_length) {
+void Encoder::EncodeAlwaysIndexed(uint32_t* index, absl::string_view key,
+                                  Slice value, size_t) {
   if (compressor_->table_.ConvertableToDynamicIndex(*index)) {
     EmitIndexed(compressor_->table_.DynamicIndex(*index));
   } else {
-    *index = compressor_->table_.AllocateIndex(transport_length);
-    EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice::FromStaticString(key),
-                                           std::move(value));
+    *index = EmitLitHdrWithNonBinaryStringKeyIncIdx(
+        Slice::FromStaticString(key), std::move(value));
   }
 }
 
-void HPackCompressor::Encoder::EncodeIndexedKeyWithBinaryValue(
-    uint32_t* index, absl::string_view key, Slice value) {
+void Encoder::EncodeIndexedKeyWithBinaryValue(uint32_t* index,
+                                              absl::string_view key,
+                                              Slice value) {
   if (compressor_->table_.ConvertableToDynamicIndex(*index)) {
     EmitLitHdrWithBinaryStringKeyNotIdx(
         compressor_->table_.DynamicIndex(*index), std::move(value));
   } else {
-    *index = compressor_->table_.AllocateIndex(key.length() + value.length() +
-                                               hpack_constants::kEntryOverhead);
-    EmitLitHdrWithBinaryStringKeyIncIdx(Slice::FromStaticString(key),
-                                        std::move(value));
+    *index = EmitLitHdrWithBinaryStringKeyIncIdx(Slice::FromStaticString(key),
+                                                 std::move(value));
   }
 }
 
-void HPackCompressor::Encoder::EncodeRepeatingSliceValue(
-    const absl::string_view& key, const Slice& slice, uint32_t* index,
-    size_t max_compression_size) {
+void Encoder::EncodeRepeatingSliceValue(const absl::string_view& key,
+                                        const Slice& slice, uint32_t* index,
+                                        size_t max_compression_size) {
   if (hpack_constants::SizeForEntry(key.size(), slice.size()) >
       max_compression_size) {
     EmitLitHdrWithBinaryStringKeyNotIdx(Slice::FromStaticString(key),
@@ -474,141 +473,37 @@ void HPackCompressor::Encoder::EncodeRepeatingSliceValue(
   }
 }
 
-void HPackCompressor::Encoder::Encode(GrpcTimeoutMetadata, Timestamp deadline) {
+void TimeoutCompressorImpl::EncodeWith(absl::string_view key,
+                                       Timestamp deadline, Encoder* encoder) {
   Timeout timeout = Timeout::FromDuration(deadline - Timestamp::Now());
-  for (auto it = compressor_->previous_timeouts_.begin();
-       it != compressor_->previous_timeouts_.end(); ++it) {
+  auto& table = encoder->hpack_table();
+  for (auto it = previous_timeouts_.begin(); it != previous_timeouts_.end();
+       ++it) {
     double ratio = timeout.RatioVersus(it->timeout);
     // If the timeout we're sending is shorter than a previous timeout, but
     // within 3% of it, we'll consider sending it.
     if (ratio > -3 && ratio <= 0 &&
-        compressor_->table_.ConvertableToDynamicIndex(it->index)) {
-      EmitIndexed(compressor_->table_.DynamicIndex(it->index));
+        table.ConvertableToDynamicIndex(it->index)) {
+      encoder->EmitIndexed(table.DynamicIndex(it->index));
       // Put this timeout to the front of the queue - forces common timeouts to
       // be considered earlier.
-      std::swap(*it, *compressor_->previous_timeouts_.begin());
+      std::swap(*it, *previous_timeouts_.begin());
       return;
     }
   }
   // Clean out some expired timeouts.
-  while (!compressor_->previous_timeouts_.empty() &&
-         !compressor_->table_.ConvertableToDynamicIndex(
-             compressor_->previous_timeouts_.back().index)) {
-    compressor_->previous_timeouts_.pop_back();
+  while (!previous_timeouts_.empty() &&
+         !table.ConvertableToDynamicIndex(previous_timeouts_.back().index)) {
+    previous_timeouts_.pop_back();
   }
   Slice encoded = timeout.Encode();
-  uint32_t index = compressor_->table_.AllocateIndex(
-      GrpcTimeoutMetadata::key().length() + encoded.length() +
-      hpack_constants::kEntryOverhead);
-  compressor_->previous_timeouts_.push_back(PreviousTimeout{timeout, index});
-  EmitLitHdrWithNonBinaryStringKeyIncIdx(
-      Slice::FromStaticString(GrpcTimeoutMetadata::key()), std::move(encoded));
+  uint32_t index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
+      Slice::FromStaticString(key), std::move(encoded));
+  previous_timeouts_.push_back(PreviousTimeout{timeout, index});
 }
 
-void HPackCompressor::Encoder::Encode(UserAgentMetadata, const Slice& slice) {
-  if (hpack_constants::SizeForEntry(UserAgentMetadata::key().size(),
-                                    slice.size()) >
-      HPackEncoderTable::MaxEntrySize()) {
-    EmitLitHdrWithNonBinaryStringKeyNotIdx(
-        Slice::FromStaticString(UserAgentMetadata::key()), slice.Ref());
-    return;
-  }
-  if (!slice.is_equivalent(compressor_->user_agent_)) {
-    compressor_->user_agent_ = slice.Ref();
-    compressor_->user_agent_index_ = 0;
-  }
-  EncodeAlwaysIndexed(&compressor_->user_agent_index_, UserAgentMetadata::key(),
-                      slice.Ref(),
-                      hpack_constants::SizeForEntry(
-                          UserAgentMetadata::key().size(), slice.size()));
-}
-
-void HPackCompressor::Encoder::Encode(GrpcStatusMetadata,
-                                      grpc_status_code status) {
-  const uint32_t code = static_cast<uint32_t>(status);
-  uint32_t* index = nullptr;
-  if (code < kNumCachedGrpcStatusValues) {
-    index = &compressor_->cached_grpc_status_[code];
-    if (compressor_->table_.ConvertableToDynamicIndex(*index)) {
-      EmitIndexed(compressor_->table_.DynamicIndex(*index));
-      return;
-    }
-  }
-  Slice key = Slice::FromStaticString(GrpcStatusMetadata::key());
-  Slice value = Slice::FromInt64(code);
-  const size_t transport_length =
-      key.length() + value.length() + hpack_constants::kEntryOverhead;
-  if (index != nullptr) {
-    *index = compressor_->table_.AllocateIndex(transport_length);
-    EmitLitHdrWithNonBinaryStringKeyIncIdx(std::move(key), std::move(value));
-  } else {
-    EmitLitHdrWithNonBinaryStringKeyNotIdx(std::move(key), std::move(value));
-  }
-}
-
-void HPackCompressor::Encoder::Encode(GrpcEncodingMetadata,
-                                      grpc_compression_algorithm value) {
-  uint32_t* index = nullptr;
-  if (value < GRPC_COMPRESS_ALGORITHMS_COUNT) {
-    index = &compressor_->cached_grpc_encoding_[static_cast<uint32_t>(value)];
-    if (compressor_->table_.ConvertableToDynamicIndex(*index)) {
-      EmitIndexed(compressor_->table_.DynamicIndex(*index));
-      return;
-    }
-  }
-  auto key = Slice::FromStaticString(GrpcEncodingMetadata::key());
-  auto encoded_value = GrpcEncodingMetadata::Encode(value);
-  size_t transport_length =
-      key.length() + encoded_value.length() + hpack_constants::kEntryOverhead;
-  if (index != nullptr) {
-    *index = compressor_->table_.AllocateIndex(transport_length);
-    EmitLitHdrWithNonBinaryStringKeyIncIdx(std::move(key),
-                                           std::move(encoded_value));
-  } else {
-    EmitLitHdrWithNonBinaryStringKeyNotIdx(std::move(key),
-                                           std::move(encoded_value));
-  }
-}
-
-void HPackCompressor::Encoder::Encode(GrpcAcceptEncodingMetadata,
-                                      CompressionAlgorithmSet value) {
-  if (compressor_->grpc_accept_encoding_index_ != 0 &&
-      value == compressor_->grpc_accept_encoding_ &&
-      compressor_->table_.ConvertableToDynamicIndex(
-          compressor_->grpc_accept_encoding_index_)) {
-    EmitIndexed(compressor_->table_.DynamicIndex(
-        compressor_->grpc_accept_encoding_index_));
-    return;
-  }
-  auto key = Slice::FromStaticString(GrpcAcceptEncodingMetadata::key());
-  auto encoded_value = GrpcAcceptEncodingMetadata::Encode(value);
-  size_t transport_length =
-      key.length() + encoded_value.length() + hpack_constants::kEntryOverhead;
-  compressor_->grpc_accept_encoding_index_ =
-      compressor_->table_.AllocateIndex(transport_length);
-  compressor_->grpc_accept_encoding_ = value;
-  EmitLitHdrWithNonBinaryStringKeyIncIdx(std::move(key),
-                                         std::move(encoded_value));
-}
-
-void HPackCompressor::SetMaxUsableSize(uint32_t max_table_size) {
-  max_usable_size_ = max_table_size;
-  SetMaxTableSize(std::min(table_.max_size(), max_table_size));
-}
-
-void HPackCompressor::SetMaxTableSize(uint32_t max_table_size) {
-  if (table_.SetMaxSize(std::min(max_usable_size_, max_table_size))) {
-    advertise_table_size_change_ = true;
-    if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace)) {
-      gpr_log(GPR_INFO, "set max table size from encoder to %d",
-              max_table_size);
-    }
-  }
-}
-
-HPackCompressor::Encoder::Encoder(HPackCompressor* compressor,
-                                  bool use_true_binary_metadata,
-                                  SliceBuffer& output)
+Encoder::Encoder(HPackCompressor* compressor, bool use_true_binary_metadata,
+                 SliceBuffer& output)
     : use_true_binary_metadata_(use_true_binary_metadata),
       compressor_(compressor),
       output_(output) {
@@ -617,4 +512,5 @@ HPackCompressor::Encoder::Encoder(HPackCompressor* compressor,
   }
 }
 
+}  // namespace hpack_encoder_detail
 }  // namespace grpc_core

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

@@ -28,15 +28,14 @@
 #include <vector>
 
 #include "absl/strings/match.h"
+#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 
-#include <grpc/impl/compression_types.h>
 #include <grpc/slice.h>
-#include <grpc/status.h>
+#include <grpc/support/log.h>
 
 #include "src/core/ext/transport/chttp2/transport/hpack_constants.h"
 #include "src/core/ext/transport/chttp2/transport/hpack_encoder_table.h"
-#include "src/core/lib/compression/compression_internal.h"
 #include "src/core/lib/gprpp/time.h"
 #include "src/core/lib/slice/slice.h"
 #include "src/core/lib/slice/slice_buffer.h"
@@ -46,6 +45,280 @@
 
 namespace grpc_core {
 
+// Forward decl for encoder
+class HPackCompressor;
+
+namespace hpack_encoder_detail {
+
+class Encoder {
+ public:
+  Encoder(HPackCompressor* compressor, bool use_true_binary_metadata,
+          SliceBuffer& output);
+
+  void Encode(const Slice& key, const Slice& value);
+  template <typename MetadataTrait>
+  void Encode(MetadataTrait, const typename MetadataTrait::ValueType& value);
+
+  void AdvertiseTableSizeChange();
+  void EmitIndexed(uint32_t index);
+  GRPC_MUST_USE_RESULT
+  uint32_t EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice key_slice,
+                                                  Slice value_slice);
+  GRPC_MUST_USE_RESULT
+  uint32_t EmitLitHdrWithBinaryStringKeyIncIdx(Slice key_slice,
+                                               Slice value_slice);
+  void EmitLitHdrWithBinaryStringKeyNotIdx(Slice key_slice, Slice value_slice);
+  void EmitLitHdrWithBinaryStringKeyNotIdx(uint32_t key_index,
+                                           Slice value_slice);
+  void EmitLitHdrWithNonBinaryStringKeyNotIdx(Slice key_slice,
+                                              Slice value_slice);
+
+  void EncodeAlwaysIndexed(uint32_t* index, absl::string_view key, Slice value,
+                           size_t transport_length);
+  void EncodeIndexedKeyWithBinaryValue(uint32_t* index, absl::string_view key,
+                                       Slice value);
+
+  void EncodeRepeatingSliceValue(const absl::string_view& key,
+                                 const Slice& slice, uint32_t* index,
+                                 size_t max_compression_size);
+
+  HPackEncoderTable& hpack_table();
+
+ private:
+  const bool use_true_binary_metadata_;
+  HPackCompressor* const compressor_;
+  SliceBuffer& output_;
+};
+
+// Compressor is partially specialized on CompressionTraits, but leaves
+// MetadataTrait as variable.
+// Via MetadataMap::StatefulCompressor it builds compression state for
+// HPackCompressor.
+// Each trait compressor gets to have some persistent state across the channel
+// (declared as Compressor member variables).
+// The compressors expose a single method:
+// void EncodeWith(MetadataTrait, const MetadataTrait::ValueType, Encoder*);
+// This method figures out how to encode the value, and then delegates to
+// Encoder to perform the encoding.
+template <typename MetadataTrait, typename CompressonTraits>
+class Compressor;
+
+// No compression encoder: just emit the key and value as literals.
+template <typename MetadataTrait>
+class Compressor<MetadataTrait, NoCompressionCompressor> {
+ public:
+  void EncodeWith(MetadataTrait, const typename MetadataTrait::ValueType& value,
+                  Encoder* encoder) {
+    const Slice& slice = MetadataValueAsSlice<MetadataTrait>(value);
+    if (absl::EndsWith(MetadataTrait::key(), "-bin")) {
+      encoder->EmitLitHdrWithBinaryStringKeyNotIdx(
+          Slice::FromStaticString(MetadataTrait::key()), slice.Ref());
+    } else {
+      encoder->EmitLitHdrWithNonBinaryStringKeyNotIdx(
+          Slice::FromStaticString(MetadataTrait::key()), slice.Ref());
+    }
+  }
+};
+
+// Frequent key with no value compression encoder
+template <typename MetadataTrait>
+class Compressor<MetadataTrait, FrequentKeyWithNoValueCompressionCompressor> {
+ public:
+  void EncodeWith(MetadataTrait, const typename MetadataTrait::ValueType& value,
+                  Encoder* encoder) {
+    const Slice& slice = MetadataValueAsSlice<MetadataTrait>(value);
+    encoder->EncodeRepeatingSliceValue(MetadataTrait::key(), slice,
+                                       &some_sent_value_,
+                                       HPackEncoderTable::MaxEntrySize());
+  }
+
+ private:
+  // Some previously sent value with this tag.
+  uint32_t some_sent_value_ = 0;
+};
+
+// Helper to determine if two objects have the same identity.
+// Equivalent here => equality, but equality does not imply equivalency.
+// For example, two slices with the same contents are equal, but not
+// equivalent.
+// Used as a much faster check for equality than the full equality check,
+// since many metadatum that are stable have the same root object in metadata
+// maps.
+template <typename T>
+static bool IsEquivalent(T a, T b) {
+  return a == b;
+}
+
+template <typename T>
+static bool IsEquivalent(const Slice& a, const Slice& b) {
+  return a.is_equivalent(b);
+}
+
+template <typename T>
+static void SaveCopyTo(const T& value, T& copy) {
+  copy = value;
+}
+
+static inline void SaveCopyTo(const Slice& value, Slice& copy) {
+  copy = value.Ref();
+}
+
+template <typename MetadataTrait>
+class Compressor<MetadataTrait, StableValueCompressor> {
+ public:
+  void EncodeWith(MetadataTrait, const typename MetadataTrait::ValueType& value,
+                  Encoder* encoder) {
+    auto& table = encoder->hpack_table();
+    if (previously_sent_value_ == value &&
+        table.ConvertableToDynamicIndex(previously_sent_index_)) {
+      encoder->EmitIndexed(table.DynamicIndex(previously_sent_index_));
+      return;
+    }
+    previously_sent_index_ = 0;
+    auto key = MetadataTrait::key();
+    const Slice& value_slice = MetadataValueAsSlice<MetadataTrait>(value);
+    if (hpack_constants::SizeForEntry(key.size(), value_slice.size()) >
+        HPackEncoderTable::MaxEntrySize()) {
+      encoder->EmitLitHdrWithNonBinaryStringKeyNotIdx(
+          Slice::FromStaticString(key), value_slice.Ref());
+      return;
+    }
+    encoder->EncodeAlwaysIndexed(
+        &previously_sent_index_, key, value_slice.Ref(),
+        hpack_constants::SizeForEntry(key.size(), value_slice.size()));
+    SaveCopyTo(value, previously_sent_value_);
+  }
+
+ private:
+  // Previously sent value
+  typename MetadataTrait::ValueType previously_sent_value_{};
+  // And its index in the table
+  uint32_t previously_sent_index_ = 0;
+};
+
+template <typename MetadataTrait, typename MetadataTrait::ValueType known_value>
+class Compressor<
+    MetadataTrait,
+    KnownValueCompressor<typename MetadataTrait::ValueType, known_value>> {
+ public:
+  void EncodeWith(MetadataTrait, const typename MetadataTrait::ValueType& value,
+                  Encoder* encoder) {
+    if (value != known_value) {
+      gpr_log(GPR_ERROR, "%s",
+              absl::StrCat("Not encoding bad ", MetadataTrait::key(), " header")
+                  .c_str());
+      return;
+    }
+    Slice encoded(MetadataTrait::Encode(known_value));
+    const auto encoded_length = encoded.length();
+    encoder->EncodeAlwaysIndexed(&previously_sent_index_, MetadataTrait::key(),
+                                 std::move(encoded),
+                                 MetadataTrait::key().size() + encoded_length +
+                                     hpack_constants::kEntryOverhead);
+  }
+
+ private:
+  uint32_t previously_sent_index_ = 0;
+};
+template <typename MetadataTrait, size_t N>
+class Compressor<MetadataTrait, SmallIntegralValuesCompressor<N>> {
+ public:
+  void EncodeWith(MetadataTrait, const typename MetadataTrait::ValueType& value,
+                  Encoder* encoder) {
+    uint32_t* index = nullptr;
+    auto& table = encoder->hpack_table();
+    if (static_cast<size_t>(value) < N) {
+      index = &previously_sent_[static_cast<uint32_t>(value)];
+      if (table.ConvertableToDynamicIndex(*index)) {
+        encoder->EmitIndexed(table.DynamicIndex(*index));
+        return;
+      }
+    }
+    auto key = Slice::FromStaticString(MetadataTrait::key());
+    auto encoded_value = MetadataTrait::Encode(value);
+    if (index != nullptr) {
+      *index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
+          std::move(key), std::move(encoded_value));
+    } else {
+      encoder->EmitLitHdrWithNonBinaryStringKeyNotIdx(std::move(key),
+                                                      std::move(encoded_value));
+    }
+  }
+
+ private:
+  uint32_t previously_sent_[N] = {};
+};
+
+class SliceIndex {
+ public:
+  void EmitTo(absl::string_view key, const Slice& value, Encoder* encoder);
+
+ private:
+  struct ValueIndex {
+    ValueIndex(Slice value, uint32_t index)
+        : value(std::move(value)), index(index) {}
+    Slice value;
+    uint32_t index;
+  };
+  std::vector<ValueIndex> values_;
+};
+
+template <typename MetadataTrait>
+class Compressor<MetadataTrait, SmallSetOfValuesCompressor> {
+ public:
+  void EncodeWith(MetadataTrait, const Slice& value, Encoder* encoder) {
+    index_.EmitTo(MetadataTrait::key(), value, encoder);
+  }
+
+ private:
+  SliceIndex index_;
+};
+
+struct PreviousTimeout {
+  Timeout timeout;
+  uint32_t index;
+};
+
+class TimeoutCompressorImpl {
+ public:
+  void EncodeWith(absl::string_view key, Timestamp deadline, Encoder* encoder);
+
+ private:
+  std::vector<PreviousTimeout> previous_timeouts_;
+};
+
+template <typename MetadataTrait>
+class Compressor<MetadataTrait, TimeoutCompressor>
+    : public TimeoutCompressorImpl {
+ public:
+  void EncodeWith(MetadataTrait, const typename MetadataTrait::ValueType& value,
+                  Encoder* encoder) {
+    TimeoutCompressorImpl::EncodeWith(MetadataTrait::key(), value, encoder);
+  }
+};
+
+template <>
+class Compressor<HttpStatusMetadata, HttpStatusCompressor> {
+ public:
+  void EncodeWith(HttpStatusMetadata, uint32_t status, Encoder* encoder);
+};
+
+template <>
+class Compressor<HttpMethodMetadata, HttpMethodCompressor> {
+ public:
+  void EncodeWith(HttpMethodMetadata, HttpMethodMetadata::ValueType method,
+                  Encoder* encoder);
+};
+
+template <>
+class Compressor<HttpSchemeMetadata, HttpSchemeCompressor> {
+ public:
+  void EncodeWith(HttpSchemeMetadata, HttpSchemeMetadata::ValueType value,
+                  Encoder* encoder);
+};
+
+}  // namespace hpack_encoder_detail
+
 class HPackCompressor {
   class SliceIndex;
 
@@ -75,87 +348,22 @@ class HPackCompressor {
   void EncodeHeaders(const EncodeHeaderOptions& options,
                      const HeaderSet& headers, grpc_slice_buffer* output) {
     SliceBuffer raw;
-    Encoder encoder(this, options.use_true_binary_metadata, raw);
+    hpack_encoder_detail::Encoder encoder(
+        this, options.use_true_binary_metadata, raw);
     headers.Encode(&encoder);
     Frame(options, raw, output);
   }
 
   template <typename HeaderSet>
   void EncodeRawHeaders(const HeaderSet& headers, SliceBuffer& output) {
-    Encoder encoder(this, true, output);
+    hpack_encoder_detail::Encoder encoder(this, true, output);
     headers.Encode(&encoder);
   }
 
  private:
-  class Encoder {
-   public:
-    Encoder(HPackCompressor* compressor, bool use_true_binary_metadata,
-            SliceBuffer& output);
-
-    void Encode(const Slice& key, const Slice& value);
-    void Encode(HttpPathMetadata, const Slice& value);
-    void Encode(HttpAuthorityMetadata, const Slice& value);
-    void Encode(HttpStatusMetadata, uint32_t status);
-    void Encode(GrpcTimeoutMetadata, Timestamp deadline);
-    void Encode(TeMetadata, TeMetadata::ValueType value);
-    void Encode(ContentTypeMetadata, ContentTypeMetadata::ValueType value);
-    void Encode(HttpSchemeMetadata, HttpSchemeMetadata::ValueType value);
-    void Encode(HttpMethodMetadata, HttpMethodMetadata::ValueType method);
-    void Encode(UserAgentMetadata, const Slice& slice);
-    void Encode(GrpcStatusMetadata, grpc_status_code status);
-    void Encode(GrpcEncodingMetadata, grpc_compression_algorithm value);
-    void Encode(GrpcAcceptEncodingMetadata, CompressionAlgorithmSet value);
-    void Encode(GrpcTagsBinMetadata, const Slice& slice);
-    void Encode(GrpcTraceBinMetadata, const Slice& slice);
-    void Encode(GrpcMessageMetadata, const Slice& slice) {
-      if (slice.empty()) return;
-      EmitLitHdrWithNonBinaryStringKeyNotIdx(
-          Slice::FromStaticString("grpc-message"), slice.Ref());
-    }
-    template <typename Which>
-    void Encode(Which, const typename Which::ValueType& value) {
-      const Slice& slice = MetadataValueAsSlice<Which>(value);
-      if (absl::EndsWith(Which::key(), "-bin")) {
-        EmitLitHdrWithBinaryStringKeyNotIdx(
-            Slice::FromStaticString(Which::key()), slice.Ref());
-      } else {
-        EmitLitHdrWithNonBinaryStringKeyNotIdx(
-            Slice::FromStaticString(Which::key()), slice.Ref());
-      }
-    }
-
-   private:
-    friend class SliceIndex;
-
-    void AdvertiseTableSizeChange();
-    void EmitIndexed(uint32_t index);
-    void EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice key_slice,
-                                                Slice value_slice);
-    void EmitLitHdrWithBinaryStringKeyIncIdx(Slice key_slice,
-                                             Slice value_slice);
-    void EmitLitHdrWithBinaryStringKeyNotIdx(Slice key_slice,
-                                             Slice value_slice);
-    void EmitLitHdrWithBinaryStringKeyNotIdx(uint32_t key_index,
-                                             Slice value_slice);
-    void EmitLitHdrWithNonBinaryStringKeyNotIdx(Slice key_slice,
-                                                Slice value_slice);
-
-    void EncodeAlwaysIndexed(uint32_t* index, absl::string_view key,
-                             Slice value, size_t transport_length);
-    void EncodeIndexedKeyWithBinaryValue(uint32_t* index, absl::string_view key,
-                                         Slice value);
-
-    void EncodeRepeatingSliceValue(const absl::string_view& key,
-                                   const Slice& slice, uint32_t* index,
-                                   size_t max_compression_size);
-
-    const bool use_true_binary_metadata_;
-    HPackCompressor* const compressor_;
-    SliceBuffer& output_;
-  };
-
   static constexpr size_t kNumFilterValues = 64;
   static constexpr uint32_t kNumCachedGrpcStatusValues = 16;
+  friend class hpack_encoder_detail::Encoder;
 
   void Frame(const EncodeHeaderOptions& options, SliceBuffer& raw,
              grpc_slice_buffer* output);
@@ -168,50 +376,24 @@ class HPackCompressor {
   bool advertise_table_size_change_ = false;
   HPackEncoderTable table_;
 
-  class SliceIndex {
-   public:
-    void EmitTo(absl::string_view key, const Slice& value, Encoder* encoder);
-
-   private:
-    struct ValueIndex {
-      ValueIndex(Slice value, uint32_t index)
-          : value(std::move(value)), index(index) {}
-      Slice value;
-      uint32_t index;
-    };
-    std::vector<ValueIndex> values_;
-  };
-
-  struct PreviousTimeout {
-    Timeout timeout;
-    uint32_t index;
-  };
-
-  // Index into table_ for the te:trailers metadata element
-  uint32_t te_index_ = 0;
-  // Index into table_ for the content-type metadata element
-  uint32_t content_type_index_ = 0;
-  // Index into table_ for the user-agent metadata element
-  uint32_t user_agent_index_ = 0;
-  // Cached grpc-status values
-  uint32_t cached_grpc_status_[kNumCachedGrpcStatusValues] = {};
-  // Cached grpc-encoding values
-  uint32_t cached_grpc_encoding_[GRPC_COMPRESS_ALGORITHMS_COUNT] = {};
-  // Cached grpc-accept-encoding value
-  uint32_t grpc_accept_encoding_index_ = 0;
-  // The grpc-accept-encoding string referred to by grpc_accept_encoding_index_
-  CompressionAlgorithmSet grpc_accept_encoding_;
-  // Index of something that was sent with grpc-tags-bin
-  uint32_t grpc_tags_bin_index_ = 0;
-  // Index of something that was sent with grpc-trace-bin
-  uint32_t grpc_trace_bin_index_ = 0;
-  // The user-agent string referred to by user_agent_index_
-  Slice user_agent_;
-  SliceIndex path_index_;
-  SliceIndex authority_index_;
-  std::vector<PreviousTimeout> previous_timeouts_;
+  grpc_metadata_batch::StatefulCompressor<hpack_encoder_detail::Compressor>
+      compression_state_;
 };
 
+namespace hpack_encoder_detail {
+
+template <typename MetadataTrait>
+void Encoder::Encode(MetadataTrait,
+                     const typename MetadataTrait::ValueType& value) {
+  compressor_->compression_state_
+      .Compressor<MetadataTrait, typename MetadataTrait::CompressionTraits>::
+          EncodeWith(MetadataTrait(), value, this);
+}
+
+inline HPackEncoderTable& Encoder::hpack_table() { return compressor_->table_; }
+
+}  // namespace hpack_encoder_detail
+
 }  // namespace grpc_core
 
 #endif  // GRPC_SRC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_HPACK_ENCODER_H

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

@@ -23,6 +23,8 @@
 namespace grpc_core {
 
 uint32_t HPackEncoderTable::AllocateIndex(size_t element_size) {
+  GPR_DEBUG_ASSERT(element_size >= 32);
+
   uint32_t new_index = tail_remote_index_ + table_elems_ + 1;
   GPR_DEBUG_ASSERT(element_size <= MaxEntrySize());
 

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

@@ -50,6 +50,8 @@ class HPackEncoderTable {
   uint32_t max_size() const { return max_table_size_; }
   // Get the current table size
   uint32_t test_only_table_size() const { return table_size_; }
+  // Get the number of entries in the table
+  uint32_t test_only_table_elems() const { return table_elems_; }
 
   // Convert an element index into a dynamic index
   uint32_t DynamicIndex(uint32_t index) const {

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

@@ -38,7 +38,6 @@
 #include "absl/types/span.h"
 #include "absl/types/variant.h"
 
-#include <grpc/status.h>
 #include <grpc/support/log.h>
 
 #include "src/core/ext/transport/chttp2/transport/decode_huff.h"
@@ -46,9 +45,11 @@
 #include "src/core/lib/debug/stats.h"
 #include "src/core/lib/debug/stats_data.h"
 #include "src/core/lib/debug/trace.h"
+#include "src/core/lib/gprpp/crash.h"
 #include "src/core/lib/gprpp/status_helper.h"
 #include "src/core/lib/slice/slice.h"
 #include "src/core/lib/slice/slice_refcount.h"
+#include "src/core/lib/surface/validate_metadata.h"
 #include "src/core/lib/transport/parsed_metadata.h"
 
 // IWYU pragma: no_include <type_traits>
@@ -80,6 +81,40 @@ struct Base64InverseTable {
 };
 
 constexpr Base64InverseTable kBase64InverseTable;
+
+absl::Status EnsureStreamError(absl::Status error) {
+  if (error.ok()) return error;
+  return grpc_error_set_int(std::move(error), StatusIntProperty::kStreamId, 0);
+}
+
+bool IsStreamError(const absl::Status& status) {
+  intptr_t stream_id;
+  return grpc_error_get_int(status, StatusIntProperty::kStreamId, &stream_id);
+}
+
+class MetadataSizeLimitExceededEncoder {
+ public:
+  explicit MetadataSizeLimitExceededEncoder(std::string& summary)
+      : summary_(summary) {}
+
+  void Encode(const Slice& key, const Slice& value) {
+    AddToSummary(key.as_string_view(), value.size());
+  }
+
+  template <typename Key, typename Value>
+  void Encode(Key, const Value& value) {
+    AddToSummary(Key::key(), EncodedSizeOfKey(Key(), value));
+  }
+
+ private:
+  void AddToSummary(absl::string_view key,
+                    size_t value_length) GPR_ATTRIBUTE_NOINLINE {
+    absl::StrAppend(&summary_, " ", key, ":",
+                    hpack_constants::SizeForEntry(key.size(), value_length),
+                    "B");
+  }
+  std::string& summary_;
+};
 }  // namespace
 
 // Input tracks the current byte through the input data and provides it
@@ -121,7 +156,8 @@ class HPackParser::Input {
   // of stream
   absl::optional<uint8_t> Next() {
     if (end_of_stream()) {
-      return UnexpectedEOF(absl::optional<uint8_t>());
+      UnexpectedEOF();
+      return absl::optional<uint8_t>();
     }
     return *begin_++;
   }
@@ -187,7 +223,10 @@ class HPackParser::Input {
   // Parse a string prefix
   absl::optional<StringPrefix> ParseStringPrefix() {
     auto cur = Next();
-    if (!cur.has_value()) return {};
+    if (!cur.has_value()) {
+      GPR_DEBUG_ASSERT(eof_error());
+      return {};
+    }
     // Huffman if the top bit is 1
     const bool huff = (*cur & 0x80) != 0;
     // String length
@@ -195,14 +234,19 @@ class HPackParser::Input {
     if (strlen == 0x7f) {
       // all ones ==> varint string length
       auto v = ParseVarint(0x7f);
-      if (!v.has_value()) return {};
+      if (!v.has_value()) {
+        GPR_DEBUG_ASSERT(eof_error());
+        return {};
+      }
       strlen = *v;
     }
     return StringPrefix{strlen, huff};
   }
 
   // Check if we saw an EOF.. must be verified before looking at TakeError
-  bool eof_error() const { return eof_error_; }
+  bool eof_error() const {
+    return eof_error_ || (!error_.ok() && !IsStreamError(error_));
+  }
 
   // Extract the parse error, leaving the current error as NONE.
   grpc_error_handle TakeError() {
@@ -211,34 +255,33 @@ class HPackParser::Input {
     return out;
   }
 
-  // Set the current error - allows the rest of the code not to need to pass
-  // around StatusOr<> which would be prohibitive here.
-  GPR_ATTRIBUTE_NOINLINE void SetError(grpc_error_handle error) {
-    if (!error_.ok() || eof_error_) {
-      return;
-    }
-    error_ = error;
+  bool has_error() const { return !error_.ok(); }
+
+  // Set the current error - tweaks the error to include a stream id so that
+  // chttp2 does not close the connection.
+  // Intended for errors that are specific to a stream and recoverable.
+  // Callers should ensure that any hpack table updates happen.
+  GPR_ATTRIBUTE_NOINLINE void SetErrorAndContinueParsing(
+      grpc_error_handle error) {
+    GPR_ASSERT(!error.ok());
+    // StreamId is used as a signal to skip this stream but keep the connection
+    // alive
+    SetError(EnsureStreamError(std::move(error)));
+  }
+
+  // Set the current error, and skip past remaining bytes.
+  // Intended for unrecoverable errors, with the expectation that they will
+  // close the connection on return to chttp2.
+  GPR_ATTRIBUTE_NOINLINE void SetErrorAndStopParsing(grpc_error_handle error) {
+    GPR_ASSERT(!error.ok());
+    SetError(std::move(error));
     begin_ = end_;
   }
 
-  // If no error is set, set it to the value produced by error_factory.
-  // Return return_value unchanged.
-  template <typename F, typename T>
-  GPR_ATTRIBUTE_NOINLINE T MaybeSetErrorAndReturn(F error_factory,
-                                                  T return_value) {
-    if (!error_.ok() || eof_error_) return return_value;
-    error_ = error_factory();
-    begin_ = end_;
-    return return_value;
-  }
-
-  // Set the error to an unexpected eof, and return result (code golfed as this
-  // is a common case)
-  template <typename T>
-  T UnexpectedEOF(T return_value) {
-    if (!error_.ok()) return return_value;
+  // Set the error to an unexpected eof
+  void UnexpectedEOF() {
+    if (!error_.ok() && !IsStreamError(error_)) return;
     eof_error_ = true;
-    return return_value;
   }
 
   // Update the frontier - signifies we've successfully parsed another element
@@ -251,14 +294,24 @@ class HPackParser::Input {
   // Helper to set the error to out of range for ParseVarint
   absl::optional<uint32_t> ParseVarintOutOfRange(uint32_t value,
                                                  uint8_t last_byte) {
-    return MaybeSetErrorAndReturn(
-        [value, last_byte] {
-          return GRPC_ERROR_CREATE(absl::StrFormat(
-              "integer overflow in hpack integer decoding: have 0x%08x, "
-              "got byte 0x%02x on byte 5",
-              value, last_byte));
-        },
-        absl::optional<uint32_t>());
+    SetErrorAndStopParsing(absl::InternalError(absl::StrFormat(
+        "integer overflow in hpack integer decoding: have 0x%08x, "
+        "got byte 0x%02x on byte 5",
+        value, last_byte)));
+    return absl::optional<uint32_t>();
+  }
+
+  // If no error is set, set it to the given error (i.e. first error wins)
+  // Do not use this directly, instead use SetErrorAndContinueParsing or
+  // SetErrorAndStopParsing.
+  void SetError(grpc_error_handle error) {
+    if (!error_.ok() || eof_error_) {
+      if (!IsStreamError(error) && IsStreamError(error_)) {
+        error_ = std::move(error);  // connection errors dominate
+      }
+      return;
+    }
+    error_ = std::move(error);
   }
 
   // Refcount if we are backed by a slice
@@ -279,6 +332,21 @@ class HPackParser::Input {
 // management characteristics
 class HPackParser::String {
  public:
+  // ParseResult carries both a ParseStatus and the parsed string
+  struct ParseResult;
+  // Result of parsing a string
+  enum class ParseStatus {
+    // Parsed OK
+    kOk,
+    // Parse reached end of the current frame
+    kEof,
+    // Parse failed due to a huffman decode error
+    kParseHuffFailed,
+    // Parse failed due to a base64 decode error
+    kUnbase64Failed,
+  };
+
+  String() : value_(absl::Span<const uint8_t>()) {}
   String(const String&) = delete;
   String& operator=(const String&) = delete;
   String(String&& other) noexcept : value_(std::move(other.value_)) {
@@ -308,72 +376,10 @@ class HPackParser::String {
   }
 
   // Parse a non-binary string
-  static absl::optional<String> Parse(Input* input) {
-    auto pfx = input->ParseStringPrefix();
-    if (!pfx.has_value()) return {};
-    if (pfx->huff) {
-      // Huffman coded
-      std::vector<uint8_t> output;
-      auto v = ParseHuff(input, pfx->length,
-                         [&output](uint8_t c) { output.push_back(c); });
-      if (!v) return {};
-      return String(std::move(output));
-    }
-    return ParseUncompressed(input, pfx->length);
-  }
+  static ParseResult Parse(Input* input);
 
   // Parse a binary string
-  static absl::optional<String> ParseBinary(Input* input) {
-    auto pfx = input->ParseStringPrefix();
-    if (!pfx.has_value()) return {};
-    if (!pfx->huff) {
-      if (pfx->length > 0 && input->peek() == 0) {
-        // 'true-binary'
-        input->Advance(1);
-        return ParseUncompressed(input, pfx->length - 1);
-      }
-      // Base64 encoded... pull out the string, then unbase64 it
-      auto base64 = ParseUncompressed(input, pfx->length);
-      if (!base64.has_value()) return {};
-      return Unbase64(input, std::move(*base64));
-    } else {
-      // Huffman encoded...
-      std::vector<uint8_t> decompressed;
-      // State here says either we don't know if it's base64 or binary, or we do
-      // and what is it.
-      enum class State { kUnsure, kBinary, kBase64 };
-      State state = State::kUnsure;
-      auto decompressed_ok =
-          ParseHuff(input, pfx->length, [&state, &decompressed](uint8_t c) {
-            if (state == State::kUnsure) {
-              // First byte... if it's zero it's binary
-              if (c == 0) {
-                // Save the type, and skip the zero
-                state = State::kBinary;
-                return;
-              } else {
-                // Flag base64, store this value
-                state = State::kBase64;
-              }
-            }
-            // Non-first byte, or base64 first byte
-            decompressed.push_back(c);
-          });
-      if (!decompressed_ok) return {};
-      switch (state) {
-        case State::kUnsure:
-          // No bytes, empty span
-          return String(absl::Span<const uint8_t>());
-        case State::kBinary:
-          // Binary, we're done
-          return String(std::move(decompressed));
-        case State::kBase64:
-          // Base64 - unpack it
-          return Unbase64(input, String(std::move(decompressed)));
-      }
-      GPR_UNREACHABLE_CODE(abort(););
-    }
-  }
+  static ParseResult ParseBinary(Input* input);
 
  private:
   void AppendBytes(const uint8_t* data, size_t length);
@@ -385,54 +391,27 @@ class HPackParser::String {
   // Parse some huffman encoded bytes, using output(uint8_t b) to emit each
   // decoded byte.
   template <typename Out>
-  static bool ParseHuff(Input* input, uint32_t length, Out output) {
+  static ParseStatus ParseHuff(Input* input, uint32_t length, Out output) {
     // If there's insufficient bytes remaining, return now.
     if (input->remaining() < length) {
-      return input->UnexpectedEOF(false);
+      input->UnexpectedEOF();
+      GPR_DEBUG_ASSERT(input->eof_error());
+      return ParseStatus::kEof;
     }
     // Grab the byte range, and iterate through it.
     const uint8_t* p = input->cur_ptr();
     input->Advance(length);
-    return HuffDecoder<Out>(output, p, p + length).Run();
+    return HuffDecoder<Out>(output, p, p + length).Run()
+               ? ParseStatus::kOk
+               : ParseStatus::kParseHuffFailed;
   }
 
   // Parse some uncompressed string bytes.
-  static absl::optional<String> ParseUncompressed(Input* input,
-                                                  uint32_t length) {
-    // Check there's enough bytes
-    if (input->remaining() < length) {
-      return input->UnexpectedEOF(absl::optional<String>());
-    }
-    auto* refcount = input->slice_refcount();
-    auto* p = input->cur_ptr();
-    input->Advance(length);
-    if (refcount != nullptr) {
-      return String(refcount, p, p + length);
-    } else {
-      return String(absl::Span<const uint8_t>(p, length));
-    }
-  }
+  static ParseResult ParseUncompressed(Input* input, uint32_t length,
+                                       uint32_t wire_size);
 
   // Turn base64 encoded bytes into not base64 encoded bytes.
-  // Only takes input to set an error on failure.
-  static absl::optional<String> Unbase64(Input* input, String s) {
-    absl::optional<std::vector<uint8_t>> result;
-    if (auto* p = absl::get_if<Slice>(&s.value_)) {
-      result = Unbase64Loop(p->begin(), p->end());
-    }
-    if (auto* p = absl::get_if<absl::Span<const uint8_t>>(&s.value_)) {
-      result = Unbase64Loop(p->begin(), p->end());
-    }
-    if (auto* p = absl::get_if<std::vector<uint8_t>>(&s.value_)) {
-      result = Unbase64Loop(p->data(), p->data() + p->size());
-    }
-    if (!result.has_value()) {
-      return input->MaybeSetErrorAndReturn(
-          [] { return GRPC_ERROR_CREATE("illegal base64 encoding"); },
-          absl::optional<String>());
-    }
-    return String(std::move(*result));
-  }
+  static ParseResult Unbase64(String s);
 
   // Main loop for Unbase64
   static absl::optional<std::vector<uint8_t>> Unbase64Loop(const uint8_t* cur,
@@ -519,25 +498,154 @@ class HPackParser::String {
   absl::variant<Slice, absl::Span<const uint8_t>, std::vector<uint8_t>> value_;
 };
 
+struct HPackParser::String::ParseResult {
+  ParseResult() = delete;
+  ParseResult(ParseStatus status, size_t wire_size, String value)
+      : status(status), wire_size(wire_size), value(std::move(value)) {}
+  ParseStatus status;
+  size_t wire_size;
+  String value;
+};
+
+HPackParser::String::ParseResult HPackParser::String::ParseUncompressed(
+    Input* input, uint32_t length, uint32_t wire_size) {
+  // Check there's enough bytes
+  if (input->remaining() < length) {
+    input->UnexpectedEOF();
+    GPR_DEBUG_ASSERT(input->eof_error());
+    return ParseResult{ParseStatus::kEof, wire_size, String{}};
+  }
+  auto* refcount = input->slice_refcount();
+  auto* p = input->cur_ptr();
+  input->Advance(length);
+  if (refcount != nullptr) {
+    return ParseResult{ParseStatus::kOk, wire_size,
+                       String(refcount, p, p + length)};
+  } else {
+    return ParseResult{ParseStatus::kOk, wire_size,
+                       String(absl::Span<const uint8_t>(p, length))};
+  }
+}
+
+HPackParser::String::ParseResult HPackParser::String::Unbase64(String s) {
+  absl::optional<std::vector<uint8_t>> result;
+  if (auto* p = absl::get_if<Slice>(&s.value_)) {
+    result = Unbase64Loop(p->begin(), p->end());
+  }
+  if (auto* p = absl::get_if<absl::Span<const uint8_t>>(&s.value_)) {
+    result = Unbase64Loop(p->begin(), p->end());
+  }
+  if (auto* p = absl::get_if<std::vector<uint8_t>>(&s.value_)) {
+    result = Unbase64Loop(p->data(), p->data() + p->size());
+  }
+  if (!result.has_value()) {
+    return ParseResult{ParseStatus::kUnbase64Failed, s.string_view().length(),
+                       String{}};
+  }
+  return ParseResult{ParseStatus::kOk, s.string_view().length(),
+                     String(std::move(*result))};
+}
+
+HPackParser::String::ParseResult HPackParser::String::Parse(Input* input) {
+  auto pfx = input->ParseStringPrefix();
+  if (!pfx.has_value()) {
+    GPR_DEBUG_ASSERT(input->eof_error());
+    return ParseResult{ParseStatus::kEof, 0, String{}};
+  }
+  if (pfx->huff) {
+    // Huffman coded
+    std::vector<uint8_t> output;
+    ParseStatus sts = ParseHuff(input, pfx->length,
+                                [&output](uint8_t c) { output.push_back(c); });
+    size_t wire_len = output.size();
+    return ParseResult{sts, wire_len, String(std::move(output))};
+  }
+  return ParseUncompressed(input, pfx->length, pfx->length);
+}
+
+HPackParser::String::ParseResult HPackParser::String::ParseBinary(
+    Input* input) {
+  auto pfx = input->ParseStringPrefix();
+  if (!pfx.has_value()) {
+    GPR_DEBUG_ASSERT(input->eof_error());
+    return ParseResult{ParseStatus::kEof, 0, String{}};
+  }
+  if (!pfx->huff) {
+    if (pfx->length > 0 && input->peek() == 0) {
+      // 'true-binary'
+      input->Advance(1);
+      return ParseUncompressed(input, pfx->length - 1, pfx->length);
+    }
+    // Base64 encoded... pull out the string, then unbase64 it
+    auto base64 = ParseUncompressed(input, pfx->length, pfx->length);
+    if (base64.status != ParseStatus::kOk) return base64;
+    return Unbase64(std::move(base64.value));
+  } else {
+    // Huffman encoded...
+    std::vector<uint8_t> decompressed;
+    // State here says either we don't know if it's base64 or binary, or we do
+    // and what is it.
+    enum class State { kUnsure, kBinary, kBase64 };
+    State state = State::kUnsure;
+    auto sts =
+        ParseHuff(input, pfx->length, [&state, &decompressed](uint8_t c) {
+          if (state == State::kUnsure) {
+            // First byte... if it's zero it's binary
+            if (c == 0) {
+              // Save the type, and skip the zero
+              state = State::kBinary;
+              return;
+            } else {
+              // Flag base64, store this value
+              state = State::kBase64;
+            }
+          }
+          // Non-first byte, or base64 first byte
+          decompressed.push_back(c);
+        });
+    if (sts != ParseStatus::kOk) {
+      return ParseResult{sts, 0, String{}};
+    }
+    switch (state) {
+      case State::kUnsure:
+        // No bytes, empty span
+        return ParseResult{ParseStatus::kOk, 0,
+                           String(absl::Span<const uint8_t>())};
+      case State::kBinary:
+        // Binary, we're done
+        {
+          size_t wire_len = decompressed.size();
+          return ParseResult{ParseStatus::kOk, wire_len,
+                             String(std::move(decompressed))};
+        }
+      case State::kBase64:
+        // Base64 - unpack it
+        return Unbase64(String(std::move(decompressed)));
+    }
+    GPR_UNREACHABLE_CODE(abort(););
+  }
+}
+
 // Parser parses one key/value pair from a byte stream.
 class HPackParser::Parser {
  public:
   Parser(Input* input, grpc_metadata_batch* metadata_buffer, HPackTable* table,
          uint8_t* dynamic_table_updates_allowed, uint32_t* frame_length,
-         RandomEarlyDetection* metadata_early_detection, bool is_last,
-         LogInfo log_info)
+         RandomEarlyDetection* metadata_early_detection, LogInfo log_info)
       : input_(input),
         metadata_buffer_(metadata_buffer),
         table_(table),
         dynamic_table_updates_allowed_(dynamic_table_updates_allowed),
         frame_length_(frame_length),
         metadata_early_detection_(metadata_early_detection),
-        is_last_(is_last),
         log_info_(log_info) {}
 
   // Skip any priority bits, or return false on failure
   bool SkipPriority() {
-    if (input_->remaining() < 5) return input_->UnexpectedEOF(false);
+    if (input_->remaining() < 5) {
+      input_->UnexpectedEOF();
+      return false;
+    }
     input_->Advance(5);
     return true;
   }
@@ -610,8 +718,9 @@ class HPackParser::Parser {
       case 8:
         if (cur == 0x80) {
           // illegal value.
-          return input_->MaybeSetErrorAndReturn(
-              [] { return GRPC_ERROR_CREATE("Illegal hpack op code"); }, false);
+          input_->SetErrorAndStopParsing(
+              absl::InternalError("Illegal hpack op code"));
+          return false;
         }
         ABSL_FALLTHROUGH_INTENDED;
       case 9:
@@ -648,24 +757,31 @@ class HPackParser::Parser {
         type = "???";
         break;
     }
-    gpr_log(GPR_DEBUG, "HTTP:%d:%s:%s: %s", log_info_.stream_id, type,
-            log_info_.is_client ? "CLI" : "SVR", memento.DebugString().c_str());
+    gpr_log(GPR_DEBUG, "HTTP:%d:%s:%s: %s%s", log_info_.stream_id, type,
+            log_info_.is_client ? "CLI" : "SVR",
+            memento.md.DebugString().c_str(),
+            memento.parse_status.ok()
+                ? ""
+                : absl::StrCat(
+                      " (parse error: ", memento.parse_status.ToString(), ")")
+                      .c_str());
   }
 
-  bool EmitHeader(const HPackTable::Memento& md) {
+  void EmitHeader(const HPackTable::Memento& md) {
     // Pass up to the transport
-    if (GPR_UNLIKELY(metadata_buffer_ == nullptr)) return true;
-    *frame_length_ += md.transport_size();
-    if (metadata_early_detection_->MustReject(*frame_length_)) {
+    *frame_length_ += md.md.transport_size();
+    if (!input_->has_error() &&
+        metadata_early_detection_->MustReject(*frame_length_)) {
       // Reject any requests above hard metadata limit.
-      return HandleMetadataSizeLimitExceeded(md, /*exceeded_hard_limit=*/true);
-    } else if (is_last_ && metadata_early_detection_->Reject(*frame_length_)) {
-      // Reject some random sample of requests above soft metadata limit.
-      return HandleMetadataSizeLimitExceeded(md, /*exceeded_hard_limit=*/false);
+      HandleMetadataHardSizeLimitExceeded(md);
+    }
+    if (!md.parse_status.ok()) {
+      // Reject any requests with invalid metadata.
+      HandleMetadataParseError(md.parse_status);
+    }
+    if (GPR_LIKELY(metadata_buffer_ != nullptr)) {
+      metadata_buffer_->Set(md.md);
     }
-
-    metadata_buffer_->Set(md);
-    return true;
   }
 
   bool FinishHeaderAndAddToTable(absl::optional<HPackTable::Memento> md) {
@@ -676,73 +792,149 @@ class HPackParser::Parser {
       LogHeader(*md);
     }
     // Emit whilst we own the metadata.
-    auto r = EmitHeader(*md);
+    EmitHeader(*md);
     // Add to the hpack table
     grpc_error_handle err = table_->Add(std::move(*md));
     if (GPR_UNLIKELY(!err.ok())) {
-      input_->SetError(err);
+      input_->SetErrorAndStopParsing(std::move(err));
       return false;
     };
-    return r;
+    return true;
   }
 
   bool FinishHeaderOmitFromTable(absl::optional<HPackTable::Memento> md) {
     // Allow higher code to just pass in failures ... simplifies things a bit.
     if (!md.has_value()) return false;
-    return FinishHeaderOmitFromTable(*md);
+    FinishHeaderOmitFromTable(*md);
+    return true;
   }
 
-  bool FinishHeaderOmitFromTable(const HPackTable::Memento& md) {
+  void FinishHeaderOmitFromTable(const HPackTable::Memento& md) {
     // Log if desired
     if (GRPC_TRACE_FLAG_ENABLED(grpc_trace_chttp2_hpack_parser)) {
       LogHeader(md);
     }
-    return EmitHeader(md);
+    EmitHeader(md);
   }
 
+  // Helper type to build a memento from a key & value, and to consolidate some
+  // tricky error path code.
+  class MementoBuilder {
+   public:
+    explicit MementoBuilder(Input* input, absl::string_view key_string,
+                            absl::Status status = absl::OkStatus())
+        : input_(input), key_string_(key_string), status_(std::move(status)) {}
+
+    auto ErrorHandler() {
+      return [this](absl::string_view error, const Slice&) {
+        auto message =
+            absl::StrCat("Error parsing '", key_string_,
+                         "' metadata: error=", error, " key=", key_string_);
+        gpr_log(GPR_ERROR, "%s", message.c_str());
+        if (status_.ok()) {
+          status_ = absl::InternalError(message);
+        }
+      };
+    }
+
+    HPackTable::Memento Build(ParsedMetadata<grpc_metadata_batch> memento) {
+      return HPackTable::Memento{std::move(memento), std::move(status_)};
+    }
+
+    // Handle the result of parsing a value.
+    // Returns true if parsing should continue, false if it should stop.
+    // Stores an error on the input if necessary.
+    bool HandleParseResult(String::ParseStatus status) {
+      auto continuable = [this](absl::string_view error) {
+        auto this_error = absl::InternalError(absl::StrCat(
+            "Error parsing '", key_string_, "' metadata: error=", error));
+        if (status_.ok()) status_ = this_error;
+        input_->SetErrorAndContinueParsing(std::move(this_error));
+      };
+      switch (status) {
+        case String::ParseStatus::kOk:
+          return true;
+        case String::ParseStatus::kParseHuffFailed:
+          input_->SetErrorAndStopParsing(
+              absl::InternalError("Huffman decoding failed"));
+          return false;
+        case String::ParseStatus::kUnbase64Failed:
+          continuable("illegal base64 encoding");
+          return true;
+        case String::ParseStatus::kEof:
+          GPR_DEBUG_ASSERT(input_->eof_error());
+          return false;
+      }
+      GPR_UNREACHABLE_CODE(return false);
+    }
+
+   private:
+    Input* input_;
+    absl::string_view key_string_;
+    absl::Status status_;
+  };
+
   // Parse a string encoded key and a string encoded value
   absl::optional<HPackTable::Memento> ParseLiteralKey() {
     auto key = String::Parse(input_);
-    if (!key.has_value()) return {};
-    auto value = ParseValueString(absl::EndsWith(key->string_view(), "-bin"));
-    if (GPR_UNLIKELY(!value.has_value())) {
-      return {};
+    switch (key.status) {
+      case String::ParseStatus::kOk:
+        break;
+      case String::ParseStatus::kParseHuffFailed:
+        input_->SetErrorAndStopParsing(
+            absl::InternalError("Huffman decoding failed"));
+        return absl::nullopt;
+      case String::ParseStatus::kUnbase64Failed:
+        Crash("unreachable");
+      case String::ParseStatus::kEof:
+        GPR_DEBUG_ASSERT(input_->eof_error());
+        return absl::nullopt;
     }
-    auto key_string = key->string_view();
-    auto value_slice = value->Take();
-    const auto transport_size = key_string.size() + value_slice.size() +
-                                hpack_constants::kEntryOverhead;
-    return grpc_metadata_batch::Parse(
-        key->string_view(), std::move(value_slice), transport_size,
-        [key_string](absl::string_view error, const Slice& value) {
-          ReportMetadataParseError(key_string, error, value.as_string_view());
-        });
+    auto key_string = key.value.string_view();
+    auto value = ParseValueString(absl::EndsWith(key_string, "-bin"));
+    MementoBuilder builder(input_, key_string,
+                           EnsureStreamError(ValidateKey(key_string)));
+    if (!builder.HandleParseResult(value.status)) return absl::nullopt;
+    auto value_slice = value.value.Take();
+    const auto transport_size =
+        key_string.size() + value.wire_size + hpack_constants::kEntryOverhead;
+    return builder.Build(
+        grpc_metadata_batch::Parse(key_string, std::move(value_slice),
+                                   transport_size, builder.ErrorHandler()));
+  }
+
+  absl::Status ValidateKey(absl::string_view key) {
+    if (key == HttpSchemeMetadata::key() || key == HttpMethodMetadata::key() ||
+        key == HttpAuthorityMetadata::key() || key == HttpPathMetadata::key() ||
+        key == HttpStatusMetadata::key()) {
+      return absl::OkStatus();
+    }
+    return ValidateHeaderKeyIsLegal(key);
   }
 
   // Parse an index encoded key and a string encoded value
   absl::optional<HPackTable::Memento> ParseIdxKey(uint32_t index) {
     const auto* elem = table_->Lookup(index);
     if (GPR_UNLIKELY(elem == nullptr)) {
-      return InvalidHPackIndexError(index,
-                                    absl::optional<HPackTable::Memento>());
+      InvalidHPackIndexError(index);
+      return absl::optional<HPackTable::Memento>();
     }
-    auto value = ParseValueString(elem->is_binary_header());
-    if (GPR_UNLIKELY(!value.has_value())) return {};
-    return elem->WithNewValue(
-        value->Take(), [=](absl::string_view error, const Slice& value) {
-          ReportMetadataParseError(elem->key(), error, value.as_string_view());
-        });
-  }
+    MementoBuilder builder(input_, elem->md.key(), elem->parse_status);
+    auto value = ParseValueString(elem->md.is_binary_header());
+    if (!builder.HandleParseResult(value.status)) return absl::nullopt;
+    return builder.Build(elem->md.WithNewValue(
+        value.value.Take(), value.wire_size, builder.ErrorHandler()));
+  };
 
   // Parse a varint index encoded key and a string encoded value
   absl::optional<HPackTable::Memento> ParseVarIdxKey(uint32_t offset) {
     auto index = input_->ParseVarint(offset);
-    if (GPR_UNLIKELY(!index.has_value())) return {};
+    if (GPR_UNLIKELY(!index.has_value())) return absl::nullopt;
     return ParseIdxKey(*index);
   }
 
   // Parse a string, figuring out if it's binary or not by the key name.
-  absl::optional<String> ParseValueString(bool is_binary) {
+  String::ParseResult ParseValueString(bool is_binary) {
     if (is_binary) {
       return String::ParseBinary(input_);
     } else {
@@ -756,26 +948,25 @@ class HPackParser::Parser {
     if (!index.has_value()) return false;
     const auto* elem = table_->Lookup(*index);
     if (GPR_UNLIKELY(elem == nullptr)) {
-      return InvalidHPackIndexError(*index, false);
+      InvalidHPackIndexError(*index);
+      return false;
     }
-    return FinishHeaderOmitFromTable(*elem);
+    FinishHeaderOmitFromTable(*elem);
+    return true;
   }
 
   // finish parsing a max table size change
   bool FinishMaxTableSize(absl::optional<uint32_t> size) {
     if (!size.has_value()) return false;
     if (*dynamic_table_updates_allowed_ == 0) {
-      return input_->MaybeSetErrorAndReturn(
-          [] {
-            return GRPC_ERROR_CREATE(
-                "More than two max table size changes in a single frame");
-          },
-          false);
+      input_->SetErrorAndStopParsing(absl::InternalError(
+          "More than two max table size changes in a single frame"));
+      return false;
     }
     (*dynamic_table_updates_allowed_)--;
     grpc_error_handle err = table_->SetCurrentTableSize(*size);
     if (!err.ok()) {
-      input_->SetError(err);
+      input_->SetErrorAndStopParsing(std::move(err));
       return false;
     }
     return true;
@@ -783,47 +974,28 @@ class HPackParser::Parser {
 
   // Set an invalid hpack index error if no error has been set. Returns result
   // unmodified.
-  template <typename R>
-  R InvalidHPackIndexError(uint32_t index, R result) {
-    return input_->MaybeSetErrorAndReturn(
-        [this, index] {
-          return grpc_error_set_int(
-              grpc_error_set_int(
-                  GRPC_ERROR_CREATE("Invalid HPACK index received"),
-                  StatusIntProperty::kIndex, static_cast<intptr_t>(index)),
-              StatusIntProperty::kSize,
-              static_cast<intptr_t>(this->table_->num_entries()));
-        },
-        std::move(result));
+  void InvalidHPackIndexError(uint32_t index) {
+    input_->SetErrorAndStopParsing(grpc_error_set_int(
+        grpc_error_set_int(absl::InternalError("Invalid HPACK index received"),
+                           StatusIntProperty::kIndex,
+                           static_cast<intptr_t>(index)),
+        StatusIntProperty::kSize,
+        static_cast<intptr_t>(this->table_->num_entries())));
   }
 
-  class MetadataSizeLimitExceededEncoder {
-   public:
-    explicit MetadataSizeLimitExceededEncoder(std::string& summary)
-        : summary_(summary) {}
-
-    void Encode(const Slice& key, const Slice& value) {
-      AddToSummary(key.as_string_view(), value.size());
+  GPR_ATTRIBUTE_NOINLINE
+  void HandleMetadataParseError(const absl::Status& status) {
+    if (metadata_buffer_ != nullptr) {
+      metadata_buffer_->Clear();
+      metadata_buffer_ = nullptr;
     }
-
-    template <typename Key, typename Value>
-    void Encode(Key, const Value& value) {
-      AddToSummary(Key::key(), EncodedSizeOfKey(Key(), value));
-    }
-
-   private:
-    void AddToSummary(absl::string_view key,
-                      size_t value_length) GPR_ATTRIBUTE_NOINLINE {
-      absl::StrAppend(&summary_, " ", key, ":",
-                      hpack_constants::SizeForEntry(key.size(), value_length),
-                      "B");
-    }
-    std::string& summary_;
-  };
+    // StreamId is used as a signal to skip this stream but keep the connection
+    // alive
+    input_->SetErrorAndContinueParsing(status);
+  }
 
   GPR_ATTRIBUTE_NOINLINE
-  bool HandleMetadataSizeLimitExceeded(const HPackTable::Memento& md,
-                                       bool exceeded_hard_limit) {
+  void HandleMetadataHardSizeLimitExceeded(const HPackTable::Memento& md) {
     // Collect a summary of sizes so far for debugging
     // Do not collect contents, for fear of exposing PII.
     std::string summary;
@@ -832,49 +1004,22 @@ class HPackParser::Parser {
       MetadataSizeLimitExceededEncoder encoder(summary);
       metadata_buffer_->Encode(&encoder);
     }
-    summary =
-        absl::StrCat("; adding ", md.key(), " (length ", md.transport_size(),
-                     "B)", summary.empty() ? "" : " to ", summary);
-    if (exceeded_hard_limit) {
-      error_message = absl::StrCat(
-          "received initial metadata size exceeds hard limit (", *frame_length_,
-          " vs. ", metadata_early_detection_->hard_limit(), ")", summary);
-    } else {
-      error_message = absl::StrCat(
-          "received initial metadata size exceeds soft limit (", *frame_length_,
-          " vs. ", metadata_early_detection_->soft_limit(),
-          "), rejecting requests with some random probability", summary);
-    }
-    if (metadata_buffer_ != nullptr) metadata_buffer_->Clear();
-    // StreamId is used as a signal to skip this stream but keep the connection
-    // alive
-    return input_->MaybeSetErrorAndReturn(
-        [error_message = std::move(error_message)] {
-          return grpc_error_set_int(
-              grpc_error_set_int(GRPC_ERROR_CREATE(error_message),
-                                 StatusIntProperty::kRpcStatus,
-                                 GRPC_STATUS_RESOURCE_EXHAUSTED),
-              StatusIntProperty::kStreamId, 0);
-        },
-        false);
-  }
-
-  static void ReportMetadataParseError(absl::string_view key,
-                                       absl::string_view error,
-                                       absl::string_view value) {
-    gpr_log(
-        GPR_ERROR, "Error parsing metadata: %s",
-        absl::StrCat("error=", error, " key=", key, " value=", value).c_str());
+    summary = absl::StrCat("; adding ", md.md.key(), " (length ",
+                           md.md.transport_size(), "B)",
+                           summary.empty() ? "" : " to ", summary);
+    error_message = absl::StrCat(
+        "received metadata size exceeds hard limit (", *frame_length_, " vs. ",
+        metadata_early_detection_->hard_limit(), ")", summary);
+    HandleMetadataParseError(absl::ResourceExhaustedError(error_message));
   }
 
   Input* const input_;
-  grpc_metadata_batch* const metadata_buffer_;
+  grpc_metadata_batch* metadata_buffer_;
   HPackTable* const table_;
   uint8_t* const dynamic_table_updates_allowed_;
   uint32_t* const frame_length_;
   // Random early detection of metadata size limits.
   RandomEarlyDetection* metadata_early_detection_;
-  bool is_last_;  // Whether this is the last frame.
   const LogInfo log_info_;
 };
 
@@ -928,26 +1073,35 @@ grpc_error_handle HPackParser::Parse(const grpc_slice& slice, bool is_last) {
 }
 
 grpc_error_handle HPackParser::ParseInput(Input input, bool is_last) {
-  bool parsed_ok = ParseInputInner(&input, is_last);
-  if (is_last) global_stats().IncrementHttp2MetadataSize(frame_length_);
-  if (parsed_ok) return absl::OkStatus();
+  ParseInputInner(&input);
+  if (is_last) {
+    if (metadata_early_detection_.Reject(frame_length_)) {
+      HandleMetadataSoftSizeLimitExceeded(&input);
+    }
+    global_stats().IncrementHttp2MetadataSize(frame_length_);
+  }
   if (input.eof_error()) {
     if (GPR_UNLIKELY(is_last && is_boundary())) {
-      return GRPC_ERROR_CREATE(
+      auto err = input.TakeError();
+      if (!err.ok() && !IsStreamError(err)) return err;
+      return absl::InternalError(
           "Incomplete header at the end of a header/continuation sequence");
     }
     unparsed_bytes_ = std::vector<uint8_t>(input.frontier(), input.end_ptr());
-    return absl::OkStatus();
+    return input.TakeError();
   }
   return input.TakeError();
 }
 
-bool HPackParser::ParseInputInner(Input* input, bool is_last) {
+void HPackParser::ParseInputInner(Input* input) {
   switch (priority_) {
     case Priority::None:
       break;
     case Priority::Included: {
-      if (input->remaining() < 5) return input->UnexpectedEOF(false);
+      if (input->remaining() < 5) {
+        input->UnexpectedEOF();
+        return;
+      }
       input->Advance(5);
       input->UpdateFrontier();
       priority_ = Priority::None;
@@ -956,15 +1110,35 @@ bool HPackParser::ParseInputInner(Input* input, bool is_last) {
   while (!input->end_of_stream()) {
     if (GPR_UNLIKELY(!Parser(input, metadata_buffer_, &table_,
                              &dynamic_table_updates_allowed_, &frame_length_,
-                             &metadata_early_detection_, is_last, log_info_)
+                             &metadata_early_detection_, log_info_)
                           .Parse())) {
-      return false;
+      return;
     }
     input->UpdateFrontier();
   }
-  return true;
 }
 
 void HPackParser::FinishFrame() { metadata_buffer_ = nullptr; }
 
+void HPackParser::HandleMetadataSoftSizeLimitExceeded(Input* input) {
+  // Collect a summary of sizes so far for debugging
+  // Do not collect contents, for fear of exposing PII.
+  std::string summary;
+  std::string error_message;
+  if (metadata_buffer_ != nullptr) {
+    MetadataSizeLimitExceededEncoder encoder(summary);
+    metadata_buffer_->Encode(&encoder);
+  }
+  error_message = absl::StrCat(
+      "received metadata size exceeds soft limit (", frame_length_, " vs. ",
+      metadata_early_detection_.soft_limit(),
+      "), rejecting requests with some random probability", summary);
+  if (metadata_buffer_ != nullptr) {
+    metadata_buffer_->Clear();
+    metadata_buffer_ = nullptr;
+  }
+  input->SetErrorAndContinueParsing(
+      absl::ResourceExhaustedError(error_message));
+}
+
 }  // namespace grpc_core

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

@@ -105,7 +105,9 @@ class HPackParser {
   class String;
 
   grpc_error_handle ParseInput(Input input, bool is_last);
-  bool ParseInputInner(Input* input, bool is_last);
+  void ParseInputInner(Input* input);
+  GPR_ATTRIBUTE_NOINLINE
+  void HandleMetadataSoftSizeLimitExceeded(Input* input);
 
   // Target metadata buffer
   grpc_metadata_batch* metadata_buffer_ = nullptr;

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

@@ -82,8 +82,8 @@ void HPackTable::MementoRingBuffer::Rebuild(uint32_t max_entries) {
 // Evict one element from the table
 void HPackTable::EvictOne() {
   auto first_entry = entries_.PopOne();
-  GPR_ASSERT(first_entry.transport_size() <= mem_used_);
-  mem_used_ -= first_entry.transport_size();
+  GPR_ASSERT(first_entry.md.transport_size() <= mem_used_);
+  mem_used_ -= first_entry.md.transport_size();
 }
 
 void HPackTable::SetMaxBytes(uint32_t max_bytes) {
@@ -104,7 +104,7 @@ grpc_error_handle HPackTable::SetCurrentTableSize(uint32_t bytes) {
     return absl::OkStatus();
   }
   if (bytes > max_bytes_) {
-    return GRPC_ERROR_CREATE(absl::StrFormat(
+    return absl::InternalError(absl::StrFormat(
         "Attempt to make hpack table %d bytes when max is %d bytes", bytes,
         max_bytes_));
   }
@@ -130,7 +130,7 @@ grpc_error_handle HPackTable::Add(Memento md) {
   }
 
   // we can't add elements bigger than the max table size
-  if (md.transport_size() > current_table_bytes_) {
+  if (md.md.transport_size() > current_table_bytes_) {
     // HPACK draft 10 section 4.4 states:
     // If the size of the new entry is less than or equal to the maximum
     // size, that entry is added to the table.  It is not an error to
@@ -145,13 +145,13 @@ grpc_error_handle HPackTable::Add(Memento md) {
   }
 
   // evict entries to ensure no overflow
-  while (md.transport_size() >
+  while (md.md.transport_size() >
          static_cast<size_t>(current_table_bytes_) - mem_used_) {
     EvictOne();
   }
 
   // copy the finalized entry in
-  mem_used_ += md.transport_size();
+  mem_used_ += md.md.transport_size();
   entries_.Put(std::move(md));
   return absl::OkStatus();
 }
@@ -228,12 +228,14 @@ const StaticTableEntry kStaticTable[hpack_constants::kLastStaticEntry] = {
 
 HPackTable::Memento MakeMemento(size_t i) {
   auto sm = kStaticTable[i];
-  return grpc_metadata_batch::Parse(
-      sm.key, Slice::FromStaticString(sm.value),
-      strlen(sm.key) + strlen(sm.value) + hpack_constants::kEntryOverhead,
-      [](absl::string_view, const Slice&) {
-        abort();  // not expecting to see this
-      });
+  return HPackTable::Memento{
+      grpc_metadata_batch::Parse(
+          sm.key, Slice::FromStaticString(sm.value),
+          strlen(sm.key) + strlen(sm.value) + hpack_constants::kEntryOverhead,
+          [](absl::string_view, const Slice&) {
+            abort();  // not expecting to see this
+          }),
+      absl::OkStatus()};
 }
 
 }  // namespace

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

@@ -25,6 +25,8 @@
 
 #include <vector>
 
+#include "absl/status/status.h"
+
 #include "src/core/ext/transport/chttp2/transport/hpack_constants.h"
 #include "src/core/lib/gprpp/no_destruct.h"
 #include "src/core/lib/iomgr/error.h"
@@ -45,7 +47,10 @@ class HPackTable {
   void SetMaxBytes(uint32_t max_bytes);
   grpc_error_handle SetCurrentTableSize(uint32_t bytes);
 
-  using Memento = ParsedMetadata<grpc_metadata_batch>;
+  struct Memento {
+    ParsedMetadata<grpc_metadata_batch> md;
+    absl::Status parse_status;
+  };
 
   // Lookup, but don't ref.
   const Memento* Lookup(uint32_t index) const {
@@ -68,6 +73,9 @@ class HPackTable {
   // Current entry count in the table.
   uint32_t num_entries() const { return entries_.num_entries(); }
 
+  // Current size of the table.
+  uint32_t test_only_table_size() const { return mem_used_; }
+
  private:
   struct StaticMementos {
     StaticMementos();

src/core/ext/xds/xds_bootstrap.cc

@@ -25,11 +25,11 @@
 
 namespace grpc_core {
 
-// TODO(roth,apolcyn): remove this federation env var after the 1.55
-// release.
+// TODO(donnadionne): check to see if federation is enabled, this will be
+// removed once federation is fully integrated and enabled by default.
 bool XdsFederationEnabled() {
   auto value = GetEnv("GRPC_EXPERIMENTAL_XDS_FEDERATION");
-  if (!value.has_value()) return true;
+  if (!value.has_value()) return false;
   bool parsed_value;
   bool parse_succeeded = gpr_parse_bool_value(value->c_str(), &parsed_value);
   return parse_succeeded && parsed_value;

src/core/lib/event_engine/event_engine.cc

@@ -20,10 +20,10 @@
 namespace grpc_event_engine {
 namespace experimental {
 
-constexpr EventEngine::TaskHandle EventEngine::TaskHandle::kInvalid = {-1, -1};
-constexpr EventEngine::ConnectionHandle
-    EventEngine::ConnectionHandle::kInvalid = {-1, -1};
-constexpr EventEngine::DNSResolver::LookupTaskHandle
+const EventEngine::TaskHandle EventEngine::TaskHandle::kInvalid = {-1, -1};
+const EventEngine::ConnectionHandle EventEngine::ConnectionHandle::kInvalid = {
+    -1, -1};
+const EventEngine::DNSResolver::LookupTaskHandle
     EventEngine::DNSResolver::LookupTaskHandle::kInvalid = {-1, -1};
 
 bool operator==(const EventEngine::TaskHandle& lhs,

src/core/lib/event_engine/posix_engine/posix_engine.h

@@ -196,6 +196,7 @@ class PosixEventEngine final : public PosixEventEngineWithFdSupport,
       const DNSResolver::ResolverOptions& options) override;
   void Run(Closure* closure) override;
   void Run(absl::AnyInvocable<void()> closure) override;
+  // Caution!! The timer implementation cannot create any fds. See #20418.
   TaskHandle RunAfter(Duration when, Closure* closure) override;
   TaskHandle RunAfter(Duration when,
                       absl::AnyInvocable<void()> closure) override;

src/core/lib/event_engine/posix_engine/posix_engine_listener.cc

@@ -23,7 +23,9 @@
 #include <sys/socket.h>  // IWYU pragma: keep
 #include <unistd.h>      // IWYU pragma: keep
 
+#include <atomic>
 #include <string>
+#include <tuple>
 #include <utility>
 
 #include "absl/functional/any_invocable.h"
@@ -41,6 +43,7 @@
 #include "src/core/lib/event_engine/posix_engine/tcp_socket_utils.h"
 #include "src/core/lib/event_engine/tcp_socket_utils.h"
 #include "src/core/lib/gprpp/status_helper.h"
+#include "src/core/lib/gprpp/time.h"
 #include "src/core/lib/iomgr/socket_mutator.h"
 
 namespace grpc_event_engine {
@@ -136,6 +139,32 @@ void PosixEngineListenerImpl::AsyncConnectionAcceptor::NotifyOnAccept(
       switch (errno) {
         case EINTR:
           continue;
+        case EMFILE:
+          // When the process runs out of fds, accept4() returns EMFILE. When
+          // this happens, the connection is left in the accept queue until
+          // either a read event triggers the on_read callback, or time has
+          // passed and the accept should be re-tried regardless. This callback
+          // is not cancelled, so a spurious wakeup may occur even when there's
+          // nothing to accept. This is not a performant code path, but if an fd
+          // limit has been reached, the system is likely in an unhappy state
+          // regardless.
+          GRPC_LOG_EVERY_N_SEC(1, GPR_ERROR, "%s",
+                               "File descriptor limit reached. Retrying.");
+          handle_->NotifyOnRead(notify_on_accept_);
+          // Do not schedule another timer if one is already armed.
+          if (retry_timer_armed_.exchange(true)) return;
+          // Hold a ref while the retry timer is waiting, to prevent listener
+          // destruction and the races that would ensue.
+          Ref();
+          std::ignore =
+              engine_->RunAfter(grpc_core::Duration::Seconds(1), [this]() {
+                retry_timer_armed_.store(false);
+                if (!handle_->IsHandleShutdown()) {
+                  handle_->SetReadable();
+                }
+                Unref();
+              });
+          return;
         case EAGAIN:
         case ECONNABORTED:
           handle_->NotifyOnRead(notify_on_accept_);

src/core/lib/event_engine/posix_engine/posix_engine_listener.h

@@ -121,6 +121,9 @@ class PosixEngineListenerImpl
     ListenerSocketsContainer::ListenerSocket socket_;
     EventHandle* handle_;
     PosixEngineClosure* notify_on_accept_;
+    // Tracks the status of a backup timer to retry accept4 calls after file
+    // descriptor exhaustion.
+    std::atomic<bool> retry_timer_armed_{false};
   };
   class ListenerAsyncAcceptors : public ListenerSocketsContainer {
    public:

src/core/lib/iomgr/tcp_server_posix.cc

@@ -16,13 +16,17 @@
 //
 //
 
+#include <grpc/support/port_platform.h>
+
+#include <utility>
+
+#include <grpc/support/atm.h>
+
 // FIXME: "posix" files shouldn't be depending on _GNU_SOURCE
 #ifndef _GNU_SOURCE
 #define _GNU_SOURCE
 #endif
 
-#include <grpc/support/port_platform.h>
-
 #include "src/core/lib/iomgr/port.h"
 
 #ifdef GRPC_POSIX_SOCKET_TCP_SERVER
@@ -45,6 +49,7 @@
 
 #include <grpc/byte_buffer.h>
 #include <grpc/event_engine/endpoint_config.h>
+#include <grpc/event_engine/event_engine.h>
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 #include <grpc/support/sync.h>
@@ -75,6 +80,8 @@
 #include "src/core/lib/transport/error_utils.h"
 
 static std::atomic<int64_t> num_dropped_connections{0};
+static constexpr grpc_core::Duration kRetryAcceptWaitTime{
+    grpc_core::Duration::Seconds(1)};
 
 using ::grpc_event_engine::experimental::EndpointConfig;
 using ::grpc_event_engine::experimental::EventEngine;
@@ -339,22 +346,38 @@ static void on_read(void* arg, grpc_error_handle err) {
     if (fd < 0) {
       if (errno == EINTR) {
         continue;
-      } else if (errno == EAGAIN || errno == ECONNABORTED ||
-                 errno == EWOULDBLOCK) {
+      }
+      // When the process runs out of fds, accept4() returns EMFILE. When this
+      // happens, the connection is left in the accept queue until either a
+      // read event triggers the on_read callback, or time has passed and the
+      // accept should be re-tried regardless. This callback is not cancelled,
+      // so a spurious wakeup may occur even when there's nothing to accept.
+      // This is not a performant code path, but if an fd limit has been
+      // reached, the system is likely in an unhappy state regardless.
+      if (errno == EMFILE) {
+        GRPC_LOG_EVERY_N_SEC(1, GPR_ERROR, "%s",
+                             "File descriptor limit reached. Retrying.");
+        grpc_fd_notify_on_read(sp->emfd, &sp->read_closure);
+        if (gpr_atm_full_xchg(&sp->retry_timer_armed, true)) return;
+        grpc_timer_init(&sp->retry_timer,
+                        grpc_core::Timestamp::Now() + kRetryAcceptWaitTime,
+                        &sp->retry_closure);
+        return;
+      }
+      if (errno == EAGAIN || errno == ECONNABORTED || errno == EWOULDBLOCK) {
         grpc_fd_notify_on_read(sp->emfd, &sp->read_closure);
         return;
-      } else {
-        gpr_mu_lock(&sp->server->mu);
-        if (!sp->server->shutdown_listeners) {
-          gpr_log(GPR_ERROR, "Failed accept4: %s",
-                  grpc_core::StrError(errno).c_str());
-        } else {
-          // if we have shutdown listeners, accept4 could fail, and we
-          // needn't notify users
-        }
-        gpr_mu_unlock(&sp->server->mu);
-        goto error;
       }
+      gpr_mu_lock(&sp->server->mu);
+      if (!sp->server->shutdown_listeners) {
+        gpr_log(GPR_ERROR, "Failed accept4: %s",
+                grpc_core::StrError(errno).c_str());
+      } else {
+        // if we have shutdown listeners, accept4 could fail, and we
+        // needn't notify users
+      }
+      gpr_mu_unlock(&sp->server->mu);
+      goto error;
     }
 
     if (sp->server->memory_quota->IsMemoryPressureHigh()) {
@@ -547,6 +570,7 @@ static grpc_error_handle clone_port(grpc_tcp_listener* listener,
     sp->port_index = listener->port_index;
     sp->fd_index = listener->fd_index + count - i;
     GPR_ASSERT(sp->emfd);
+    grpc_tcp_server_listener_initialize_retry_timer(sp);
     while (listener->server->tail->next != nullptr) {
       listener->server->tail = listener->server->tail->next;
     }
@@ -780,6 +804,7 @@ static void tcp_server_shutdown_listeners(grpc_tcp_server* s) {
   if (s->active_ports) {
     grpc_tcp_listener* sp;
     for (sp = s->head; sp; sp = sp->next) {
+      grpc_timer_cancel(&sp->retry_timer);
       grpc_fd_shutdown(sp->emfd, GRPC_ERROR_CREATE("Server shutdown"));
     }
   }

src/core/lib/iomgr/tcp_server_utils_posix.h

@@ -30,6 +30,7 @@
 #include "src/core/lib/iomgr/resolve_address.h"
 #include "src/core/lib/iomgr/socket_utils_posix.h"
 #include "src/core/lib/iomgr/tcp_server.h"
+#include "src/core/lib/iomgr/timer.h"
 #include "src/core/lib/resource_quota/memory_quota.h"
 
 // one listening port
@@ -52,6 +53,11 @@ typedef struct grpc_tcp_listener {
   // identified while iterating through 'next'.
   struct grpc_tcp_listener* sibling;
   int is_sibling;
+  // If an accept4() call fails, a timer is started to drain the accept queue in
+  // case no further connection attempts reach the gRPC server.
+  grpc_closure retry_closure;
+  grpc_timer retry_timer;
+  gpr_atm retry_timer_armed;
 } grpc_tcp_listener;
 
 // the overall server
@@ -139,4 +145,10 @@ grpc_error_handle grpc_tcp_server_prepare_socket(
 // Ruturn true if the platform supports ifaddrs
 bool grpc_tcp_server_have_ifaddrs(void);
 
+// Initialize (but don't start) the timer and callback to retry accept4() on a
+// listening socket after file descriptors have been exhausted. This must be
+// called when creating a new listener.
+void grpc_tcp_server_listener_initialize_retry_timer(
+    grpc_tcp_listener* listener);
+
 #endif  // GRPC_SRC_CORE_LIB_IOMGR_TCP_SERVER_UTILS_POSIX_H

src/core/lib/iomgr/tcp_server_utils_posix_common.cc

@@ -18,6 +18,8 @@
 
 #include <grpc/support/port_platform.h>
 
+#include <grpc/support/atm.h>
+
 #include "src/core/lib/iomgr/port.h"
 
 #ifdef GRPC_POSIX_SOCKET_TCP_SERVER_UTILS_COMMON
@@ -81,6 +83,24 @@ static int get_max_accept_queue_size(void) {
   return s_max_accept_queue_size;
 }
 
+static void listener_retry_timer_cb(void* arg, grpc_error_handle err) {
+  // Do nothing if cancelled.
+  if (!err.ok()) return;
+  grpc_tcp_listener* listener = static_cast<grpc_tcp_listener*>(arg);
+  gpr_atm_no_barrier_store(&listener->retry_timer_armed, false);
+  if (!grpc_fd_is_shutdown(listener->emfd)) {
+    grpc_fd_set_readable(listener->emfd);
+  }
+}
+
+void grpc_tcp_server_listener_initialize_retry_timer(
+    grpc_tcp_listener* listener) {
+  gpr_atm_no_barrier_store(&listener->retry_timer_armed, false);
+  grpc_timer_init_unset(&listener->retry_timer);
+  GRPC_CLOSURE_INIT(&listener->retry_closure, listener_retry_timer_cb, listener,
+                    grpc_schedule_on_exec_ctx);
+}
+
 static grpc_error_handle add_socket_to_server(grpc_tcp_server* s, int fd,
                                               const grpc_resolved_address* addr,
                                               unsigned port_index,
@@ -112,6 +132,7 @@ static grpc_error_handle add_socket_to_server(grpc_tcp_server* s, int fd,
   sp->server = s;
   sp->fd = fd;
   sp->emfd = grpc_fd_create(fd, name.c_str(), true);
+  grpc_tcp_server_listener_initialize_retry_timer(sp);
 
   // Check and set fd as prellocated
   if (grpc_tcp_server_pre_allocated_fd(s) == fd) {

src/core/lib/surface/validate_metadata.cc

@@ -21,46 +21,20 @@
 #include "src/core/lib/surface/validate_metadata.h"
 
 #include "absl/status/status.h"
+#include "absl/strings/escaping.h"
+#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 
 #include <grpc/grpc.h>
 
-#include "src/core/lib/gpr/string.h"
 #include "src/core/lib/gprpp/bitset.h"
-#include "src/core/lib/gprpp/memory.h"
-#include "src/core/lib/gprpp/status_helper.h"
 #include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/slice/slice_internal.h"
 
-static grpc_error_handle conforms_to(const grpc_slice& slice,
-                                     const grpc_core::BitSet<256>& legal_bits,
-                                     const char* err_desc) {
-  const uint8_t* p = GRPC_SLICE_START_PTR(slice);
-  const uint8_t* e = GRPC_SLICE_END_PTR(slice);
-  for (; p != e; p++) {
-    if (!legal_bits.is_set(*p)) {
-      size_t len;
-      grpc_core::UniquePtr<char> ptr(gpr_dump_return_len(
-          reinterpret_cast<const char*> GRPC_SLICE_START_PTR(slice),
-          GRPC_SLICE_LENGTH(slice), GPR_DUMP_HEX | GPR_DUMP_ASCII, &len));
-      grpc_error_handle error = grpc_error_set_str(
-          grpc_error_set_int(GRPC_ERROR_CREATE(err_desc),
-                             grpc_core::StatusIntProperty::kOffset,
-                             p - GRPC_SLICE_START_PTR(slice)),
-          grpc_core::StatusStrProperty::kRawBytes,
-          absl::string_view(ptr.get(), len));
-      return error;
-    }
-  }
-  return absl::OkStatus();
-}
-
-static int error2int(grpc_error_handle error) {
-  int r = (error.ok());
-  return r;
-}
+namespace grpc_core {
 
 namespace {
-class LegalHeaderKeyBits : public grpc_core::BitSet<256> {
+class LegalHeaderKeyBits : public BitSet<256> {
  public:
   constexpr LegalHeaderKeyBits() {
     for (int i = 'a'; i <= 'z'; i++) set(i);
@@ -71,19 +45,45 @@ class LegalHeaderKeyBits : public grpc_core::BitSet<256> {
   }
 };
 constexpr LegalHeaderKeyBits g_legal_header_key_bits;
+
+GPR_ATTRIBUTE_NOINLINE
+absl::Status DoesNotConformTo(absl::string_view x, const char* err_desc) {
+  return absl::InternalError(absl::StrCat(err_desc, ": ", x, " (hex ",
+                                          absl::BytesToHexString(x), ")"));
+}
+
+absl::Status ConformsTo(absl::string_view x, const BitSet<256>& legal_bits,
+                        const char* err_desc) {
+  for (uint8_t c : x) {
+    if (!legal_bits.is_set(c)) {
+      return DoesNotConformTo(x, err_desc);
+    }
+  }
+  return absl::OkStatus();
+}
 }  // namespace
 
+absl::Status ValidateHeaderKeyIsLegal(absl::string_view key) {
+  if (key.empty()) {
+    return absl::InternalError("Metadata keys cannot be zero length");
+  }
+  if (key.size() > UINT32_MAX) {
+    return absl::InternalError(
+        "Metadata keys cannot be larger than UINT32_MAX");
+  }
+  return ConformsTo(key, g_legal_header_key_bits, "Illegal header key");
+}
+
+}  // namespace grpc_core
+
+static int error2int(grpc_error_handle error) {
+  int r = (error.ok());
+  return r;
+}
+
 grpc_error_handle grpc_validate_header_key_is_legal(const grpc_slice& slice) {
-  if (GRPC_SLICE_LENGTH(slice) == 0) {
-    return GRPC_ERROR_CREATE("Metadata keys cannot be zero length");
-  }
-  if (GRPC_SLICE_LENGTH(slice) > UINT32_MAX) {
-    return GRPC_ERROR_CREATE("Metadata keys cannot be larger than UINT32_MAX");
-  }
-  if (GRPC_SLICE_START_PTR(slice)[0] == ':') {
-    return GRPC_ERROR_CREATE("Metadata keys cannot start with :");
-  }
-  return conforms_to(slice, g_legal_header_key_bits, "Illegal header key");
+  return grpc_core::ValidateHeaderKeyIsLegal(
+      grpc_core::StringViewFromSlice(slice));
 }
 
 int grpc_header_key_is_legal(grpc_slice slice) {
@@ -104,8 +104,9 @@ constexpr LegalHeaderNonBinValueBits g_legal_header_non_bin_value_bits;
 
 grpc_error_handle grpc_validate_header_nonbin_value_is_legal(
     const grpc_slice& slice) {
-  return conforms_to(slice, g_legal_header_non_bin_value_bits,
-                     "Illegal header value");
+  return grpc_core::ConformsTo(grpc_core::StringViewFromSlice(slice),
+                               g_legal_header_non_bin_value_bits,
+                               "Illegal header value");
 }
 
 int grpc_header_nonbin_value_is_legal(grpc_slice slice) {

src/core/lib/surface/validate_metadata.h

@@ -25,11 +25,20 @@
 
 #include <cstring>
 
+#include "absl/status/status.h"
+#include "absl/strings/string_view.h"
+
 #include <grpc/slice.h>
 #include <grpc/support/log.h>
 
 #include "src/core/lib/iomgr/error.h"
 
+namespace grpc_core {
+
+absl::Status ValidateHeaderKeyIsLegal(absl::string_view key);
+
+}
+
 grpc_error_handle grpc_validate_header_key_is_legal(const grpc_slice& slice);
 grpc_error_handle grpc_validate_header_nonbin_value_is_legal(
     const grpc_slice& slice);

src/core/lib/transport/metadata_batch.h

@@ -50,6 +50,50 @@
 
 namespace grpc_core {
 
+///////////////////////////////////////////////////////////////////////////////
+// Compression traits.
+//
+// Each metadata trait exposes exactly one compression trait.
+// This type directs how transports might choose to compress the metadata.
+// Adding a value here typically involves editing all transports to support the
+// trait, and so should not be done lightly.
+
+// No compression.
+struct NoCompressionCompressor {};
+
+// Expect a single value for this metadata key, but we don't know apriori its
+// value.
+// It's ok if it changes over time, but it should be mostly stable.
+// This is used for things like user-agent, which is expected to be the same
+// for all requests.
+struct StableValueCompressor {};
+
+// Expect a single value for this metadata key, and we know apriori its value.
+template <typename T, T value>
+struct KnownValueCompressor {};
+
+// Values are uncompressible, but expect the key to be in most requests and try
+// and compress that.
+struct FrequentKeyWithNoValueCompressionCompressor {};
+
+// Expect a small set of values for this metadata key.
+struct SmallSetOfValuesCompressor {};
+
+// Expect integral values up to N for this metadata key.
+template <size_t N>
+struct SmallIntegralValuesCompressor {};
+
+// Specialty compressor for grpc-timeout metadata.
+struct TimeoutCompressor {};
+
+// Specialty compressors for HTTP/2 psuedo headers.
+struct HttpSchemeCompressor {};
+struct HttpMethodCompressor {};
+struct HttpStatusCompressor {};
+
+///////////////////////////////////////////////////////////////////////////////
+// Metadata traits
+
 // Given a metadata key and a value, return the encoded size.
 // Defaults to calling the key's Encode() method and then calculating the size
 // of that, but can be overridden for specific keys if there's a better way of
@@ -71,6 +115,7 @@ struct GrpcTimeoutMetadata {
   static constexpr bool kRepeatable = false;
   using ValueType = Timestamp;
   using MementoType = Duration;
+  using CompressionTraits = TimeoutCompressor;
   static absl::string_view key() { return "grpc-timeout"; }
   static MementoType ParseMemento(Slice value, MetadataParseErrorFn on_error);
   static ValueType MementoToValue(MementoType timeout);
@@ -90,6 +135,7 @@ struct TeMetadata {
     kInvalid,
   };
   using MementoType = ValueType;
+  using CompressionTraits = KnownValueCompressor<ValueType, kTrailers>;
   static absl::string_view key() { return "te"; }
   static MementoType ParseMemento(Slice value, MetadataParseErrorFn on_error);
   static ValueType MementoToValue(MementoType te) { return te; }
@@ -117,6 +163,7 @@ struct ContentTypeMetadata {
     kInvalid,
   };
   using MementoType = ValueType;
+  using CompressionTraits = KnownValueCompressor<ValueType, kApplicationGrpc>;
   static absl::string_view key() { return "content-type"; }
   static MementoType ParseMemento(Slice value, MetadataParseErrorFn on_error);
   static ValueType MementoToValue(MementoType content_type) {
@@ -139,6 +186,7 @@ struct HttpSchemeMetadata {
     kInvalid,
   };
   using MementoType = ValueType;
+  using CompressionTraits = HttpSchemeCompressor;
   static absl::string_view key() { return ":scheme"; }
   static MementoType ParseMemento(Slice value, MetadataParseErrorFn on_error) {
     return Parse(value.as_string_view(), on_error);
@@ -167,6 +215,7 @@ struct HttpMethodMetadata {
     kInvalid,
   };
   using MementoType = ValueType;
+  using CompressionTraits = HttpMethodCompressor;
   static absl::string_view key() { return ":method"; }
   static MementoType ParseMemento(Slice value, MetadataParseErrorFn on_error);
   static ValueType MementoToValue(MementoType content_type) {
@@ -203,12 +252,15 @@ struct CompressionAlgorithmBasedMetadata {
 // grpc-encoding metadata trait.
 struct GrpcEncodingMetadata : public CompressionAlgorithmBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits =
+      SmallIntegralValuesCompressor<GRPC_COMPRESS_ALGORITHMS_COUNT>;
   static absl::string_view key() { return "grpc-encoding"; }
 };
 
 // grpc-internal-encoding-request metadata trait.
 struct GrpcInternalEncodingRequest : public CompressionAlgorithmBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = NoCompressionCompressor;
   static absl::string_view key() { return "grpc-internal-encoding-request"; }
 };
 
@@ -218,6 +270,7 @@ struct GrpcAcceptEncodingMetadata {
   static absl::string_view key() { return "grpc-accept-encoding"; }
   using ValueType = CompressionAlgorithmSet;
   using MementoType = ValueType;
+  using CompressionTraits = StableValueCompressor;
   static MementoType ParseMemento(Slice value, MetadataParseErrorFn) {
     return CompressionAlgorithmSet::FromString(value.as_string_view());
   }
@@ -248,54 +301,63 @@ struct SimpleSliceBasedMetadata {
 // user-agent metadata trait.
 struct UserAgentMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = StableValueCompressor;
   static absl::string_view key() { return "user-agent"; }
 };
 
 // grpc-message metadata trait.
 struct GrpcMessageMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = NoCompressionCompressor;
   static absl::string_view key() { return "grpc-message"; }
 };
 
 // host metadata trait.
 struct HostMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = NoCompressionCompressor;
   static absl::string_view key() { return "host"; }
 };
 
 // endpoint-load-metrics-bin metadata trait.
 struct EndpointLoadMetricsBinMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = NoCompressionCompressor;
   static absl::string_view key() { return "endpoint-load-metrics-bin"; }
 };
 
 // grpc-server-stats-bin metadata trait.
 struct GrpcServerStatsBinMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = NoCompressionCompressor;
   static absl::string_view key() { return "grpc-server-stats-bin"; }
 };
 
 // grpc-trace-bin metadata trait.
 struct GrpcTraceBinMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = FrequentKeyWithNoValueCompressionCompressor;
   static absl::string_view key() { return "grpc-trace-bin"; }
 };
 
 // grpc-tags-bin metadata trait.
 struct GrpcTagsBinMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = FrequentKeyWithNoValueCompressionCompressor;
   static absl::string_view key() { return "grpc-tags-bin"; }
 };
 
 // :authority metadata trait.
 struct HttpAuthorityMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = SmallSetOfValuesCompressor;
   static absl::string_view key() { return ":authority"; }
 };
 
 // :path metadata trait.
 struct HttpPathMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = SmallSetOfValuesCompressor;
   static absl::string_view key() { return ":path"; }
 };
 
@@ -329,6 +391,7 @@ struct SimpleIntBasedMetadata : public SimpleIntBasedMetadataBase<Int> {
 struct GrpcStatusMetadata
     : public SimpleIntBasedMetadata<grpc_status_code, GRPC_STATUS_UNKNOWN> {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = SmallIntegralValuesCompressor<16>;
   static absl::string_view key() { return "grpc-status"; }
 };
 
@@ -336,6 +399,7 @@ struct GrpcStatusMetadata
 struct GrpcPreviousRpcAttemptsMetadata
     : public SimpleIntBasedMetadata<uint32_t, 0> {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = NoCompressionCompressor;
   static absl::string_view key() { return "grpc-previous-rpc-attempts"; }
 };
 
@@ -345,6 +409,7 @@ struct GrpcRetryPushbackMsMetadata {
   static absl::string_view key() { return "grpc-retry-pushback-ms"; }
   using ValueType = Duration;
   using MementoType = Duration;
+  using CompressionTraits = NoCompressionCompressor;
   static ValueType MementoToValue(MementoType x) { return x; }
   static Slice Encode(Duration x) { return Slice::FromInt64(x.millis()); }
   static int64_t DisplayValue(Duration x) { return x.millis(); }
@@ -356,6 +421,7 @@ struct GrpcRetryPushbackMsMetadata {
 // TODO(ctiller): consider moving to uint16_t
 struct HttpStatusMetadata : public SimpleIntBasedMetadata<uint32_t, 0> {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = HttpStatusCompressor;
   static absl::string_view key() { return ":status"; }
 };
 
@@ -368,6 +434,7 @@ struct GrpcLbClientStatsMetadata {
   static absl::string_view key() { return "grpclb_client_stats"; }
   using ValueType = GrpcLbClientStats*;
   using MementoType = ValueType;
+  using CompressionTraits = NoCompressionCompressor;
   static ValueType MementoToValue(MementoType value) { return value; }
   static Slice Encode(ValueType) { abort(); }
   static const char* DisplayValue(ValueType) { return "<internal-lb-stats>"; }
@@ -387,6 +454,7 @@ inline size_t EncodedSizeOfKey(GrpcLbClientStatsMetadata,
 // lb-token metadata
 struct LbTokenMetadata : public SimpleSliceBasedMetadata {
   static constexpr bool kRepeatable = false;
+  using CompressionTraits = NoCompressionCompressor;
   static absl::string_view key() { return "lb-token"; }
 };
 
@@ -399,6 +467,7 @@ struct LbCostBinMetadata {
     std::string name;
   };
   using MementoType = ValueType;
+  using CompressionTraits = NoCompressionCompressor;
   static ValueType MementoToValue(MementoType value) { return value; }
   static Slice Encode(const ValueType& x);
   static std::string DisplayValue(ValueType x);
@@ -576,8 +645,9 @@ class ParseHelper {
 
   GPR_ATTRIBUTE_NOINLINE ParsedMetadata<Container> NotFound(
       absl::string_view key) {
-    return ParsedMetadata<Container>(Slice::FromCopiedString(key),
-                                     std::move(value_));
+    return ParsedMetadata<Container>(
+        typename ParsedMetadata<Container>::FromSlicePair{},
+        Slice::FromCopiedString(key), std::move(value_), transport_size_);
   }
 
  private:
@@ -993,6 +1063,35 @@ class UnknownMap {
   ChunkedVector<std::pair<Slice, Slice>, 10> unknown_;
 };
 
+// Given a factory template Factory, construct a type that derives from
+// Factory<MetadataTrait, MetadataTrait::CompressionTraits> for all
+// MetadataTraits. Useful for transports in defining the stateful parts of their
+// compression algorithm.
+template <template <typename, typename> class Factory,
+          typename... MetadataTraits>
+struct StatefulCompressor;
+
+template <template <typename, typename> class Factory, typename MetadataTrait,
+          bool kEncodable = IsEncodableTrait<MetadataTrait>::value>
+struct SpecificStatefulCompressor;
+
+template <template <typename, typename> class Factory, typename MetadataTrait>
+struct SpecificStatefulCompressor<Factory, MetadataTrait, true>
+    : public Factory<MetadataTrait, typename MetadataTrait::CompressionTraits> {
+};
+
+template <template <typename, typename> class Factory, typename MetadataTrait>
+struct SpecificStatefulCompressor<Factory, MetadataTrait, false> {};
+
+template <template <typename, typename> class Factory, typename MetadataTrait,
+          typename... MetadataTraits>
+struct StatefulCompressor<Factory, MetadataTrait, MetadataTraits...>
+    : public SpecificStatefulCompressor<Factory, MetadataTrait>,
+      public StatefulCompressor<Factory, MetadataTraits...> {};
+
+template <template <typename, typename> class Factory>
+struct StatefulCompressor<Factory> {};
+
 }  // namespace metadata_detail
 
 // Helper function for encoders
@@ -1111,6 +1210,15 @@ class MetadataMap {
   explicit MetadataMap(Arena* arena);
   ~MetadataMap();
 
+  // Given a compressor factory - template taking <MetadataTrait,
+  // CompressionTrait>, StatefulCompressor<Factory> provides a type
+  // derived from all Encodable traits in this MetadataMap.
+  // This can be used by transports to delegate compression to the appropriate
+  // compression algorithm.
+  template <template <typename, typename> class Factory>
+  using StatefulCompressor =
+      metadata_detail::StatefulCompressor<Factory, Traits...>;
+
   MetadataMap(const MetadataMap&) = delete;
   MetadataMap& operator=(const MetadataMap&) = delete;
   MetadataMap(MetadataMap&&) noexcept;

src/core/lib/transport/parsed_metadata.h

@@ -26,6 +26,7 @@
 
 #include "absl/functional/function_ref.h"
 #include "absl/meta/type_traits.h"
+#include "absl/strings/escaping.h"
 #include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
@@ -151,9 +152,14 @@ class ParsedMetadata {
     value_.slice = value.TakeCSlice();
   }
   // Construct metadata from a string key, slice value pair.
-  ParsedMetadata(Slice key, Slice value)
+  // FromSlicePair() is used to adjust the overload set so that we don't
+  // inadvertently match against any of the previous overloads.
+  // TODO(ctiller): re-evaluate the overload functions here so and maybe
+  // introduce some factory functions?
+  struct FromSlicePair {};
+  ParsedMetadata(FromSlicePair, Slice key, Slice value, uint32_t transport_size)
       : vtable_(ParsedMetadata::KeyValueVTable(key.as_string_view())),
-        transport_size_(static_cast<uint32_t>(key.size() + value.size() + 32)) {
+        transport_size_(transport_size) {
     value_.pointer =
         new std::pair<Slice, Slice>(std::move(key), std::move(value));
   }
@@ -187,14 +193,13 @@ class ParsedMetadata {
   // HTTP2 defined storage size of this metadatum.
   uint32_t transport_size() const { return transport_size_; }
   // Create a new parsed metadata with the same key but a different value.
-  ParsedMetadata WithNewValue(Slice value,
+  ParsedMetadata WithNewValue(Slice value, uint32_t value_wire_size,
                               MetadataParseErrorFn on_error) const {
     ParsedMetadata result;
     result.vtable_ = vtable_;
     result.value_ = value_;
     result.transport_size_ =
-        TransportSize(static_cast<uint32_t>(key().length()),
-                      static_cast<uint32_t>(value.length()));
+        TransportSize(static_cast<uint32_t>(key().length()), value_wire_size);
     vtable_->with_new_value(&value, on_error, &result);
     return result;
   }
@@ -395,12 +400,17 @@ ParsedMetadata<MetadataContainer>::KeyValueVTable(absl::string_view key) {
     return absl::StrCat(p->first.as_string_view(), ": ",
                         p->second.as_string_view());
   };
+  static const auto binary_debug_string = [](const Buffer& value) {
+    auto* p = static_cast<KV*>(value.pointer);
+    return absl::StrCat(p->first.as_string_view(), ": \"",
+                        absl::CEscape(p->second.as_string_view()), "\"");
+  };
   static const auto key_fn = [](const Buffer& value) {
     return static_cast<KV*>(value.pointer)->first.as_string_view();
   };
   static const VTable vtable[2] = {
       {false, destroy, set, with_new_value, debug_string, "", key_fn},
-      {true, destroy, set, with_new_value, debug_string, "", key_fn},
+      {true, destroy, set, with_new_value, binary_debug_string, "", key_fn},
   };
   return &vtable[absl::EndsWith(key, "-bin")];
 }

src/cpp/common/alarm.cc

@@ -18,14 +18,8 @@
 #include <grpc/support/port_platform.h>
 
 #include <functional>
-#include <memory>
 #include <utility>
 
-#include "absl/base/thread_annotations.h"
-#include "absl/status/status.h"
-#include "absl/types/optional.h"
-
-#include <grpc/event_engine/event_engine.h>
 #include <grpc/grpc.h>
 #include <grpc/support/log.h>
 #include <grpc/support/sync.h>
@@ -34,28 +28,22 @@
 #include <grpcpp/completion_queue.h>
 #include <grpcpp/impl/completion_queue_tag.h>
 
-#include "src/core/lib/event_engine/default_event_engine.h"
-#include "src/core/lib/gprpp/sync.h"
 #include "src/core/lib/gprpp/time.h"
+#include "src/core/lib/iomgr/closure.h"
 #include "src/core/lib/iomgr/error.h"
 #include "src/core/lib/iomgr/exec_ctx.h"
+#include "src/core/lib/iomgr/executor.h"
+#include "src/core/lib/iomgr/timer.h"
 #include "src/core/lib/surface/completion_queue.h"
 
 namespace grpc {
 
 namespace internal {
-
-namespace {
-using ::grpc_event_engine::experimental::EventEngine;
-}  // namespace
-
 class AlarmImpl : public grpc::internal::CompletionQueueTag {
  public:
-  AlarmImpl()
-      : event_engine_(grpc_event_engine::experimental::GetDefaultEventEngine()),
-        cq_(nullptr),
-        tag_(nullptr) {
+  AlarmImpl() : cq_(nullptr), tag_(nullptr) {
     gpr_ref_init(&refs_, 1);
+    grpc_timer_init_unset(&timer_);
   }
   ~AlarmImpl() override {}
   bool FinalizeResult(void** tag, bool* /*status*/) override {
@@ -64,46 +52,61 @@ class AlarmImpl : public grpc::internal::CompletionQueueTag {
     return true;
   }
   void Set(grpc::CompletionQueue* cq, gpr_timespec deadline, void* tag) {
-    grpc_core::MutexLock lock(&mu_);
-    GPR_ASSERT(!cq_timer_handle_.has_value() &&
-               !callback_timer_handle_.has_value());
+    grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
     grpc_core::ExecCtx exec_ctx;
     GRPC_CQ_INTERNAL_REF(cq->cq(), "alarm");
     cq_ = cq->cq();
     tag_ = tag;
     GPR_ASSERT(grpc_cq_begin_op(cq_, this));
-    Ref();
-    cq_timer_handle_ = event_engine_->RunAfter(
-        grpc_core::Timestamp::FromTimespecRoundUp(deadline) -
-            grpc_core::ExecCtx::Get()->Now(),
-        [this] { OnCQAlarm(absl::OkStatus()); });
+    GRPC_CLOSURE_INIT(
+        &on_alarm_,
+        [](void* arg, grpc_error_handle error) {
+          // queue the op on the completion queue
+          AlarmImpl* alarm = static_cast<AlarmImpl*>(arg);
+          alarm->Ref();
+          // Preserve the cq and reset the cq_ so that the alarm
+          // can be reset when the alarm tag is delivered.
+          grpc_completion_queue* cq = alarm->cq_;
+          alarm->cq_ = nullptr;
+          grpc_cq_end_op(
+              cq, alarm, error,
+              [](void* /*arg*/, grpc_cq_completion* /*completion*/) {}, arg,
+              &alarm->completion_);
+          GRPC_CQ_INTERNAL_UNREF(cq, "alarm");
+        },
+        this, grpc_schedule_on_exec_ctx);
+    grpc_timer_init(&timer_,
+                    grpc_core::Timestamp::FromTimespecRoundUp(deadline),
+                    &on_alarm_);
   }
   void Set(gpr_timespec deadline, std::function<void(bool)> f) {
-    grpc_core::MutexLock lock(&mu_);
-    GPR_ASSERT(!cq_timer_handle_.has_value() &&
-               !callback_timer_handle_.has_value());
+    grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
     grpc_core::ExecCtx exec_ctx;
     // Don't use any CQ at all. Instead just use the timer to fire the function
     callback_ = std::move(f);
     Ref();
-    callback_timer_handle_ = event_engine_->RunAfter(
-        grpc_core::Timestamp::FromTimespecRoundUp(deadline) -
-            grpc_core::ExecCtx::Get()->Now(),
-        [this] { OnCallbackAlarm(true); });
+    GRPC_CLOSURE_INIT(
+        &on_alarm_,
+        [](void* arg, grpc_error_handle error) {
+          grpc_core::Executor::Run(GRPC_CLOSURE_CREATE(
+                                       [](void* arg, grpc_error_handle error) {
+                                         AlarmImpl* alarm =
+                                             static_cast<AlarmImpl*>(arg);
+                                         alarm->callback_(error.ok());
+                                         alarm->Unref();
+                                       },
+                                       arg, nullptr),
+                                   error);
+        },
+        this, grpc_schedule_on_exec_ctx);
+    grpc_timer_init(&timer_,
+                    grpc_core::Timestamp::FromTimespecRoundUp(deadline),
+                    &on_alarm_);
   }
   void Cancel() {
+    grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
     grpc_core::ExecCtx exec_ctx;
-    grpc_core::MutexLock lock(&mu_);
-    if (callback_timer_handle_.has_value() &&
-        event_engine_->Cancel(*callback_timer_handle_)) {
-      event_engine_->Run([this] { OnCallbackAlarm(/*is_ok=*/false); });
-      callback_timer_handle_.reset();
-    } else if (cq_timer_handle_.has_value() &&
-               event_engine_->Cancel(*cq_timer_handle_)) {
-      event_engine_->Run(
-          [this] { OnCQAlarm(absl::CancelledError("cancelled")); });
-      cq_timer_handle_.reset();
-    }
+    grpc_timer_cancel(&timer_);
   }
   void Destroy() {
     Cancel();
@@ -111,35 +114,6 @@ class AlarmImpl : public grpc::internal::CompletionQueueTag {
   }
 
  private:
-  void OnCQAlarm(grpc_error_handle error) {
-    {
-      grpc_core::MutexLock lock(&mu_);
-      cq_timer_handle_.reset();
-    }
-    grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
-    grpc_core::ExecCtx exec_ctx;
-    // Preserve the cq and reset the cq_ so that the alarm
-    // can be reset when the alarm tag is delivered.
-    grpc_completion_queue* cq = cq_;
-    cq_ = nullptr;
-    grpc_cq_end_op(
-        cq, this, error,
-        [](void* /*arg*/, grpc_cq_completion* /*completion*/) {}, nullptr,
-        &completion_);
-    GRPC_CQ_INTERNAL_UNREF(cq, "alarm");
-  }
-
-  void OnCallbackAlarm(bool is_ok) {
-    {
-      grpc_core::MutexLock lock(&mu_);
-      callback_timer_handle_.reset();
-    }
-    grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
-    grpc_core::ExecCtx exec_ctx;
-    callback_(is_ok);
-    Unref();
-  }
-
   void Ref() { gpr_ref(&refs_); }
   void Unref() {
     if (gpr_unref(&refs_)) {
@@ -147,12 +121,9 @@ class AlarmImpl : public grpc::internal::CompletionQueueTag {
     }
   }
 
-  grpc_core::Mutex mu_;
-  std::shared_ptr<grpc_event_engine::experimental::EventEngine> event_engine_;
-  absl::optional<EventEngine::TaskHandle> cq_timer_handle_ ABSL_GUARDED_BY(mu_);
-  absl::optional<EventEngine::TaskHandle> callback_timer_handle_
-      ABSL_GUARDED_BY(mu_);
+  grpc_timer timer_;
   gpr_refcount refs_;
+  grpc_closure on_alarm_;
   grpc_cq_completion completion_;
   // completion queue where events about this alarm will be posted
   grpc_completion_queue* cq_;

src/cpp/ext/gcp/BUILD

@@ -40,6 +40,7 @@ grpc_cc_library(
         "absl/status",
         "absl/status:statusor",
         "absl/strings",
+        "absl/time",
         "absl/types:optional",
         "google/api:monitored_resource_cc_proto",
         "googleapis_monitoring_grpc_service",

src/cpp/ext/gcp/environment_autodetect.h

@@ -34,13 +34,10 @@
 
 namespace grpc {
 
-namespace experimental {
-// Forward declaration for GcpObservabilityInit
-absl::Status GcpObservabilityInit();
-}  // namespace experimental
-
 namespace internal {
 
+absl::Status GcpObservabilityInit();
+
 class EnvironmentAutoDetect {
  public:
   struct ResourceType {
@@ -64,7 +61,7 @@ class EnvironmentAutoDetect {
   }
 
  private:
-  friend absl::Status grpc::experimental::GcpObservabilityInit();
+  friend absl::Status grpc::internal::GcpObservabilityInit();
 
   // GcpObservabilityInit() is responsible for setting up the singleton with the
   // project_id.

src/cpp/ext/gcp/observability.cc

@@ -28,6 +28,8 @@
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
+#include "absl/time/clock.h"
+#include "absl/time/time.h"
 #include "absl/types/optional.h"
 #include "google/api/monitored_resource.pb.h"
 #include "google/devtools/cloudtrace/v2/tracing.grpc.pb.h"
@@ -45,6 +47,7 @@
 #include <grpcpp/support/channel_arguments.h>
 
 #include "src/core/ext/filters/logging/logging_filter.h"
+#include "src/core/lib/gprpp/crash.h"
 #include "src/core/lib/gprpp/notification.h"
 #include "src/cpp/client/client_stats_interceptor.h"
 #include "src/cpp/ext/filters/census/client_filter.h"
@@ -55,12 +58,14 @@
 #include "src/cpp/ext/gcp/observability_logging_sink.h"
 
 namespace grpc {
-namespace experimental {
 
+namespace internal {
 namespace {
 
 grpc::internal::ObservabilityLoggingSink* g_logging_sink = nullptr;
 
+bool g_gcp_observability_initialized = false;
+
 // TODO(yashykt): These constants are currently derived from the example at
 // https://cloud.google.com/traffic-director/docs/observability-proxyless#c++.
 // We might want these to be configurable.
@@ -74,18 +79,20 @@ constexpr char kGoogleStackdriverStatsAddress[] = "monitoring.googleapis.com";
 
 void RegisterOpenCensusViewsForGcpObservability() {
   // Register client default views for GCP observability
-  ClientStartedRpcs().RegisterForExport();
-  ClientCompletedRpcs().RegisterForExport();
-  ClientRoundtripLatency().RegisterForExport();
+  experimental::ClientStartedRpcs().RegisterForExport();
+  experimental::ClientCompletedRpcs().RegisterForExport();
+  experimental::ClientRoundtripLatency().RegisterForExport();
   internal::ClientApiLatency().RegisterForExport();
-  ClientSentCompressedMessageBytesPerRpc().RegisterForExport();
-  ClientReceivedCompressedMessageBytesPerRpc().RegisterForExport();
+  experimental::ClientSentCompressedMessageBytesPerRpc().RegisterForExport();
+  experimental::ClientReceivedCompressedMessageBytesPerRpc()
+      .RegisterForExport();
   // Register server default views for GCP observability
-  ServerStartedRpcs().RegisterForExport();
-  ServerCompletedRpcs().RegisterForExport();
-  ServerSentCompressedMessageBytesPerRpc().RegisterForExport();
-  ServerReceivedCompressedMessageBytesPerRpc().RegisterForExport();
-  ServerServerLatency().RegisterForExport();
+  experimental::ServerStartedRpcs().RegisterForExport();
+  experimental::ServerCompletedRpcs().RegisterForExport();
+  experimental::ServerSentCompressedMessageBytesPerRpc().RegisterForExport();
+  experimental::ServerReceivedCompressedMessageBytesPerRpc()
+      .RegisterForExport();
+  experimental::ServerServerLatency().RegisterForExport();
 }
 
 }  // namespace
@@ -100,6 +107,10 @@ absl::Status GcpObservabilityInit() {
       !config->cloud_logging.has_value()) {
     return absl::OkStatus();
   }
+  if (g_gcp_observability_initialized) {
+    grpc_core::Crash("GCP Observability for gRPC was already initialized.");
+  }
+  g_gcp_observability_initialized = true;
   grpc::internal::EnvironmentAutoDetect::Create(config->project_id);
   if (!config->cloud_trace.has_value()) {
     // Disable OpenCensus tracing
@@ -206,7 +217,55 @@ void GcpObservabilityClose() {
   if (g_logging_sink != nullptr) {
     g_logging_sink->FlushAndClose();
   }
+  // Currently, GcpObservabilityClose() only supports flushing logs. Stats and
+  // tracing get automatically flushed at a regular interval, so sleep for an
+  // interval to make sure that those are flushed too.
+  absl::SleepFor(absl::Seconds(25));
 }
 
+}  // namespace internal
+
+namespace experimental {
+
+absl::Status GcpObservabilityInit() {
+  return grpc::internal::GcpObservabilityInit();
+}
+
+void GcpObservabilityClose() { return grpc::internal::GcpObservabilityClose(); }
+
 }  // namespace experimental
+
+//
+// GcpObservability
+//
+
+absl::StatusOr<GcpObservability> GcpObservability::Init() {
+  absl::Status status = grpc::internal::GcpObservabilityInit();
+  if (!status.ok()) {
+    return status;
+  }
+  GcpObservability obj;
+  obj.impl_ = std::make_unique<GcpObservabilityImpl>();
+  return obj;
+}
+
+GcpObservability::GcpObservability(GcpObservability&& other) noexcept
+    : impl_(std::move(other.impl_)) {}
+
+GcpObservability& GcpObservability::operator=(
+    GcpObservability&& other) noexcept {
+  if (this != &other) {
+    impl_ = std::move(other.impl_);
+  }
+  return *this;
+}
+
+//
+// GcpObservability::GcpObservabilityImpl
+//
+
+GcpObservability::GcpObservabilityImpl::~GcpObservabilityImpl() {
+  grpc::internal::GcpObservabilityClose();
+}
+
 }  // namespace grpc

src/csharp/build/dependencies.props

@@ -1,7 +1,7 @@
 <!-- This file is generated -->
 <Project>
   <PropertyGroup>
-    <GrpcCsharpVersion>2.54.0-dev</GrpcCsharpVersion>
+    <GrpcCsharpVersion>2.54.3</GrpcCsharpVersion>
     <GoogleProtobufVersion>3.21.12</GoogleProtobufVersion>
   </PropertyGroup>
 </Project>

src/objective-c/!ProtoCompiler-gRPCCppPlugin.podspec

@@ -42,7 +42,7 @@ Pod::Spec.new do |s|
   # exclamation mark ensures that other "regular" pods will be able to find it as it'll be installed
   # before them.
   s.name     = '!ProtoCompiler-gRPCCppPlugin'
-  v = '1.54.0-dev'
+  v = '1.54.3'
   s.version  = v
   s.summary  = 'The gRPC ProtoC plugin generates C++ files from .proto services.'
   s.description = <<-DESC

src/objective-c/!ProtoCompiler-gRPCPlugin.podspec

@@ -42,7 +42,7 @@ Pod::Spec.new do |s|
   # exclamation mark ensures that other "regular" pods will be able to find it as it'll be installed
   # before them.
   s.name     = '!ProtoCompiler-gRPCPlugin'
-  v = '1.54.0-dev'
+  v = '1.54.3'
   s.version  = v
   s.summary  = 'The gRPC ProtoC plugin generates Objective-C files from .proto services.'
   s.description = <<-DESC

src/objective-c/GRPCClient/version.h

@@ -22,4 +22,4 @@
 // instead. This file can be regenerated from the template by running
 // `tools/buildgen/generate_projects.sh`.
 
-#define GRPC_OBJC_VERSION_STRING @"1.54.0-dev"
+#define GRPC_OBJC_VERSION_STRING @"1.54.3"

src/objective-c/tests/version.h

@@ -22,5 +22,5 @@
 // instead. This file can be regenerated from the template by running
 // `tools/buildgen/generate_projects.sh`.
 
-#define GRPC_OBJC_VERSION_STRING @"1.54.0-dev"
+#define GRPC_OBJC_VERSION_STRING @"1.54.3"
 #define GRPC_C_VERSION_STRING @"31.0.0"

src/php/composer.json

@@ -2,7 +2,7 @@
   "name": "grpc/grpc-dev",
   "description": "gRPC library for PHP - for Development use only",
   "license": "Apache-2.0",
-  "version": "1.54.0",
+  "version": "1.54.3",
   "require": {
     "php": ">=7.0.0",
     "google/protobuf": "^v3.3.0"

src/php/ext/grpc/version.h

@@ -20,6 +20,6 @@
 #ifndef VERSION_H
 #define VERSION_H
 
-#define PHP_GRPC_VERSION "1.54.0dev"
+#define PHP_GRPC_VERSION "1.54.3"
 
 #endif /* VERSION_H */

src/python/grpcio/grpc/_grpcio_metadata.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio/grpc/_grpcio_metadata.py.template`!!!
 
-__version__ = """1.54.0.dev0"""
+__version__ = """1.54.3"""

src/python/grpcio/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_admin/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_admin/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_channelz/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_channelz/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_csds/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_csds/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_health_checking/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_health_checking/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_reflection/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_reflection/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_status/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_status/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_testing/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_testing/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_tests/grpc_version.py

@@ -14,4 +14,4 @@
 
 # AUTO-GENERATED FROM `$REPO_ROOT/templates/src/python/grpcio_tests/grpc_version.py.template`!!!
 
-VERSION = '1.54.0.dev0'
+VERSION = '1.54.3'

src/python/grpcio_tests/tests_py3_only/interop/Dockerfile.client

@@ -1,11 +1,6 @@
-FROM phusion/baseimage:master@sha256:65ea10d5f757e5e86272625f8675d437dd83d8db64bdb429e2354d58f5462750
+FROM python:3.9-slim-bookworm
 
-RUN apt-get update -y && \
-        apt-get install -y \
-            build-essential \
-            clang \
-            python3 \
-            python3-dev
+RUN apt-get update -y && apt-get upgrade -y && apt-get install -y build-essential clang curl
 
 WORKDIR /workdir
 
@@ -16,13 +11,18 @@ COPY . .
 RUN tools/bazel build -c dbg //src/python/grpcio_tests/tests_py3_only/interop:xds_interop_client
 RUN cp -rL /workdir/bazel-bin/src/python/grpcio_tests/tests_py3_only/interop/xds_interop_client* /artifacts/
 
-FROM phusion/baseimage:master@sha256:65ea10d5f757e5e86272625f8675d437dd83d8db64bdb429e2354d58f5462750
-COPY --from=0 /artifacts ./
+FROM python:3.9-slim-bookworm
 
 ENV GRPC_VERBOSITY="DEBUG"
 ENV GRPC_TRACE="xds_client,xds_resolver,xds_cluster_manager_lb,cds_lb,xds_cluster_resolver_lb,priority_lb,xds_cluster_impl_lb,weighted_target_lb,ring_hash_lb,outlier_detection_lb"
 
-RUN apt-get update -y && apt-get install -y python3
+RUN apt-get update -y \
+  && apt-get install -y python3 \
+  && apt-get -y autoremove \
+  && rm -rf /var/lib/apt/lists/*
+
 RUN ln -s /usr/bin/python3 /usr/bin/python
 
+COPY --from=0 /artifacts ./
+
 ENTRYPOINT ["/xds_interop_client"]

src/python/grpcio_tests/tests_py3_only/interop/Dockerfile.server

@@ -1,11 +1,6 @@
-FROM phusion/baseimage:master@sha256:65ea10d5f757e5e86272625f8675d437dd83d8db64bdb429e2354d58f5462750
+FROM python:3.9-slim-bookworm
 
-RUN apt-get update -y && \
-        apt-get install -y \
-            build-essential \
-            clang \
-            python3 \
-            python3-dev
+RUN apt-get update -y && apt-get upgrade -y && apt-get install -y build-essential clang curl
 
 WORKDIR /workdir
 
@@ -16,13 +11,18 @@ COPY . .
 RUN tools/bazel build -c dbg //src/python/grpcio_tests/tests_py3_only/interop:xds_interop_server
 RUN cp -rL /workdir/bazel-bin/src/python/grpcio_tests/tests_py3_only/interop/xds_interop_server* /artifacts/
 
-FROM phusion/baseimage:master@sha256:65ea10d5f757e5e86272625f8675d437dd83d8db64bdb429e2354d58f5462750
-COPY --from=0 /artifacts ./
+FROM python:3.9-slim-bookworm
 
 ENV GRPC_VERBOSITY="DEBUG"
 ENV GRPC_TRACE="xds_client,xds_resolver,xds_cluster_manager_lb,cds_lb,xds_cluster_resolver_lb,priority_lb,xds_cluster_impl_lb,weighted_target_lb"
 
-RUN apt-get update -y && apt-get install -y python3
+RUN apt-get update -y \
+    && apt-get install -y python3 \
+    && apt-get -y autoremove \
+    && rm -rf /var/lib/apt/lists/*
+
 RUN ln -s /usr/bin/python3 /usr/bin/python
 
+COPY --from=0 /artifacts ./
+
 ENTRYPOINT ["/xds_interop_server"]

src/ruby/lib/grpc/version.rb

@@ -14,5 +14,5 @@
 
 # GRPC contains the General RPC module.
 module GRPC
-  VERSION = '1.54.0.dev'
+  VERSION = '1.54.3'
 end

src/ruby/tools/version.rb

@@ -14,6 +14,6 @@
 
 module GRPC
   module Tools
-    VERSION = '1.54.0.dev'
+    VERSION = '1.54.3'
   end
 end

test/core/end2end/tests/large_metadata.cc

@@ -207,8 +207,7 @@ static void test_request_with_large_metadata_between_soft_and_hard_limits(
     auto status = send_metadata(f.get(), metadata_size, &client_details);
     if (status == GRPC_STATUS_RESOURCE_EXHAUSTED) {
       num_requests_rejected++;
-      const char* expected_error =
-          "received initial metadata size exceeds soft limit";
+      const char* expected_error = "received metadata size exceeds soft limit";
       grpc_slice actual_error =
           grpc_slice_split_head(&client_details, strlen(expected_error));
       GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -246,8 +245,7 @@ static void test_request_with_large_metadata_above_hard_limit(
     grpc_slice client_details;
     auto status = send_metadata(f.get(), metadata_size, &client_details);
     GPR_ASSERT(status == GRPC_STATUS_RESOURCE_EXHAUSTED);
-    const char* expected_error =
-        "received initial metadata size exceeds hard limit";
+    const char* expected_error = "received metadata size exceeds hard limit";
     grpc_slice actual_error =
         grpc_slice_split_head(&client_details, strlen(expected_error));
     GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -292,8 +290,7 @@ static void test_request_with_large_metadata_soft_limit_above_hard_limit(
     auto status =
         send_metadata(f.get(), metadata_size_above_hard_limit, &client_details);
     GPR_ASSERT(status == GRPC_STATUS_RESOURCE_EXHAUSTED);
-    const char* expected_error =
-        "received initial metadata size exceeds hard limit";
+    const char* expected_error = "received metadata size exceeds hard limit";
     grpc_slice actual_error =
         grpc_slice_split_head(&client_details, strlen(expected_error));
     GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -337,8 +334,7 @@ static void test_request_with_large_metadata_soft_limit_overrides_default_hard(
         send_metadata(f.get(), metadata_size_between_limits, &client_details);
     if (status == GRPC_STATUS_RESOURCE_EXHAUSTED) {
       num_requests_rejected++;
-      const char* expected_error =
-          "received initial metadata size exceeds soft limit";
+      const char* expected_error = "received metadata size exceeds soft limit";
       grpc_slice actual_error =
           grpc_slice_split_head(&client_details, strlen(expected_error));
       GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -358,8 +354,7 @@ static void test_request_with_large_metadata_soft_limit_overrides_default_hard(
     auto status =
         send_metadata(f.get(), metadata_size_above_hard_limit, &client_details);
     GPR_ASSERT(status == GRPC_STATUS_RESOURCE_EXHAUSTED);
-    const char* expected_error =
-        "received initial metadata size exceeds hard limit";
+    const char* expected_error = "received metadata size exceeds hard limit";
     grpc_slice actual_error =
         grpc_slice_split_head(&client_details, strlen(expected_error));
     GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -404,8 +399,7 @@ static void test_request_with_large_metadata_hard_limit_overrides_default_soft(
         send_metadata(f.get(), metadata_size_between_limits, &client_details);
     if (status == GRPC_STATUS_RESOURCE_EXHAUSTED) {
       num_requests_rejected++;
-      const char* expected_error =
-          "received initial metadata size exceeds soft limit";
+      const char* expected_error = "received metadata size exceeds soft limit";
       grpc_slice actual_error =
           grpc_slice_split_head(&client_details, strlen(expected_error));
       GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -425,8 +419,7 @@ static void test_request_with_large_metadata_hard_limit_overrides_default_soft(
     auto status =
         send_metadata(f.get(), metadata_size_above_hard_limit, &client_details);
     GPR_ASSERT(status == GRPC_STATUS_RESOURCE_EXHAUSTED);
-    const char* expected_error =
-        "received initial metadata size exceeds hard limit";
+    const char* expected_error = "received metadata size exceeds hard limit";
     grpc_slice actual_error =
         grpc_slice_split_head(&client_details, strlen(expected_error));
     GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -467,8 +460,7 @@ static void test_request_with_large_metadata_hard_limit_below_default_hard(
     auto status =
         send_metadata(f.get(), metadata_size_above_hard_limit, &client_details);
     GPR_ASSERT(status == GRPC_STATUS_RESOURCE_EXHAUSTED);
-    const char* expected_error =
-        "received initial metadata size exceeds hard limit";
+    const char* expected_error = "received metadata size exceeds hard limit";
     grpc_slice actual_error =
         grpc_slice_split_head(&client_details, strlen(expected_error));
     GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -512,8 +504,7 @@ static void test_request_with_large_metadata_soft_limit_below_default_soft(
         send_metadata(f.get(), metadata_size_between_limits, &client_details);
     if (status == GRPC_STATUS_RESOURCE_EXHAUSTED) {
       num_requests_rejected++;
-      const char* expected_error =
-          "received initial metadata size exceeds soft limit";
+      const char* expected_error = "received metadata size exceeds soft limit";
       grpc_slice actual_error =
           grpc_slice_split_head(&client_details, strlen(expected_error));
       GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));
@@ -533,8 +524,7 @@ static void test_request_with_large_metadata_soft_limit_below_default_soft(
     auto status =
         send_metadata(f.get(), metadata_size_above_hard_limit, &client_details);
     GPR_ASSERT(status == GRPC_STATUS_RESOURCE_EXHAUSTED);
-    const char* expected_error =
-        "received initial metadata size exceeds hard limit";
+    const char* expected_error = "received metadata size exceeds hard limit";
     grpc_slice actual_error =
         grpc_slice_split_head(&client_details, strlen(expected_error));
     GPR_ASSERT(0 == grpc_slice_str_cmp(actual_error, expected_error));

test/core/transport/chttp2/BUILD

@@ -32,6 +32,18 @@ grpc_proto_fuzzer(
     ],
 )
 
+grpc_proto_fuzzer(
+    name = "hpack_sync_fuzzer",
+    srcs = ["hpack_sync_fuzzer.cc"],
+    corpus = "hpack_sync_corpus",
+    proto = "hpack_sync_fuzzer.proto",
+    tags = ["no_windows"],
+    deps = [
+        "//:grpc",
+        "//test/core/util:grpc_test_util",
+    ],
+)
+
 grpc_proto_fuzzer(
     name = "flow_control_fuzzer",
     srcs = ["flow_control_fuzzer.cc"],
@@ -47,6 +59,23 @@ grpc_proto_fuzzer(
     ],
 )
 
+grpc_fuzzer(
+    name = "hpack_parser_input_size_fuzzer",
+    srcs = ["hpack_parser_input_size_fuzzer.cc"],
+    corpus = "hpack_parser_input_size_corpus",
+    external_deps = [
+        "absl/cleanup",
+        "absl/status:statusor",
+        "absl/status",
+    ],
+    tags = ["no_windows"],
+    deps = [
+        "//:grpc",
+        "//test/core/util:grpc_test_util",
+        "//test/core/util:grpc_test_util_base",
+    ],
+)
+
 grpc_fuzzer(
     name = "decode_huff_fuzzer",
     srcs = ["decode_huff_fuzzer.cc"],

test/core/transport/chttp2/MiXeD-CaSe.headers

@@ -0,0 +1 @@
+MiXeD-CaSe: should not parse

test/core/transport/chttp2/bad-base64.headers

@@ -0,0 +1 @@
+a.b.c-bin: luckily for us, it's tuesday

test/core/transport/chttp2/bad-te.headers

@@ -0,0 +1 @@
+te: garbage

test/core/transport/chttp2/bin_encoder_test.cc

@@ -70,7 +70,9 @@ static void expect_combined_equiv(const char* s, size_t len, int line) {
   grpc_slice input = grpc_slice_from_copied_buffer(s, len);
   grpc_slice base64 = grpc_chttp2_base64_encode(input);
   grpc_slice expect = grpc_chttp2_huffman_compress(base64);
-  grpc_slice got = grpc_chttp2_base64_encode_and_huffman_compress(input);
+  uint32_t wire_size;
+  grpc_slice got =
+      grpc_chttp2_base64_encode_and_huffman_compress(input, &wire_size);
   if (!grpc_slice_eq(expect, got)) {
     char* t = grpc_dump_slice(input, GPR_DUMP_HEX | GPR_DUMP_ASCII);
     char* e = grpc_dump_slice(expect, GPR_DUMP_HEX | GPR_DUMP_ASCII);

test/core/transport/chttp2/hpack_encoder_test.cc

@@ -193,7 +193,7 @@ static void verify(
     bool is_eof, const char* expected,
     const std::vector<std::pair<std::string, std::string>>& header_fields) {
   const grpc_core::Slice merged(EncodeHeaderIntoBytes(is_eof, header_fields));
-  const grpc_core::Slice expect(parse_hexstring(expected));
+  const grpc_core::Slice expect(grpc_core::ParseHexstring(expected));
 
   EXPECT_EQ(merged, expect);
 }
@@ -343,6 +343,66 @@ TEST(HpackEncoderTest, TestContinuationHeaders) {
   delete g_compressor;
 }
 
+TEST(HpackEncoderTest, EncodeBinaryAsBase64) {
+  grpc_core::MemoryAllocator memory_allocator =
+      grpc_core::MemoryAllocator(grpc_core::ResourceQuota::Default()
+                                     ->memory_quota()
+                                     ->CreateMemoryAllocator("test"));
+  auto arena = grpc_core::MakeScopedArena(1024, &memory_allocator);
+  grpc_metadata_batch b(arena.get());
+  // Haiku by Bard
+  b.Append("grpc-trace-bin",
+           grpc_core::Slice::FromStaticString(
+               "Base64, a tool\nTo encode binary data into "
+               "text\nSo it can be shared."),
+           CrashOnAppendError);
+  grpc_transport_one_way_stats stats;
+  stats = {};
+  grpc_slice_buffer output;
+  grpc_slice_buffer_init(&output);
+  grpc_core::HPackCompressor::EncodeHeaderOptions hopt = {
+      0xdeadbeef,  // stream_id
+      true,        // is_eof
+      false,       // use_true_binary_metadata
+      150,         // max_frame_size
+      &stats};
+  grpc_core::HPackCompressor compressor;
+  compressor.EncodeHeaders(hopt, b, &output);
+  grpc_slice_buffer_destroy(&output);
+
+  EXPECT_EQ(compressor.test_only_table_size(), 136);
+}
+
+TEST(HpackEncoderTest, EncodeBinaryAsTrueBinary) {
+  grpc_core::MemoryAllocator memory_allocator =
+      grpc_core::MemoryAllocator(grpc_core::ResourceQuota::Default()
+                                     ->memory_quota()
+                                     ->CreateMemoryAllocator("test"));
+  auto arena = grpc_core::MakeScopedArena(1024, &memory_allocator);
+  grpc_metadata_batch b(arena.get());
+  // Haiku by Bard
+  b.Append("grpc-trace-bin",
+           grpc_core::Slice::FromStaticString(
+               "Base64, a tool\nTo encode binary data into "
+               "text\nSo it can be shared."),
+           CrashOnAppendError);
+  grpc_transport_one_way_stats stats;
+  stats = {};
+  grpc_slice_buffer output;
+  grpc_slice_buffer_init(&output);
+  grpc_core::HPackCompressor::EncodeHeaderOptions hopt = {
+      0xdeadbeef,  // stream_id
+      true,        // is_eof
+      true,        // use_true_binary_metadata
+      150,         // max_frame_size
+      &stats};
+  grpc_core::HPackCompressor compressor;
+  compressor.EncodeHeaders(hopt, b, &output);
+  grpc_slice_buffer_destroy(&output);
+
+  EXPECT_EQ(compressor.test_only_table_size(), 114);
+}
+
 int main(int argc, char** argv) {
   grpc::testing::TestEnvironment env(&argc, argv);
   ::testing::InitGoogleTest(&argc, argv);

test/core/transport/chttp2/hpack_parser_corpus/crash-39214285ae59b7193aad114858056cca7c21a8e5

@@ -0,0 +1,12 @@
+frames {
+  max_metadata_length: 4096
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin;\t!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+}
+frames {
+  max_metadata_length: 4096
+  parse: "*\244\020\007\360\244\017-bin\203c\035\037\000\'[\360i(bn-!?\244\037\333\360!(!\\\360\tc"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+  absolute_max_metadata_length: 4096
+}

test/core/transport/chttp2/hpack_parser_corpus/crash-7f5f186fa8ac3950346da51bce3d76d0437d3b20

@@ -0,0 +1,38 @@
+frames {
+  end_of_headers: true
+}
+frames {
+  end_of_headers: true
+  end_of_stream: true
+  stop_buffering_after_segments: 4096
+  max_metadata_length: 16252928
+  parse: "iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii"
+  parse: "\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272"
+  parse: "\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272"
+  parse: "\244\244\020\007\360\244\017-bin\213c*[)\244(\244-\017\333\360\'\360b\203cin\t!!"
+  absolute_max_metadata_length: 16252928
+}
+frames {
+  end_of_headers: true
+  end_of_stream: true
+  max_metadata_length: -4093
+  parse: "\261\261\261\261\261\261\261"
+  parse: "D\005:path"
+  parse: "\261\261\261\261\261\261\261"
+  parse: "\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272"
+  parse: "\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272"
+  absolute_max_metadata_length: 4096
+}
+frames {
+  parse: "\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272"
+}
+frames {
+  end_of_headers: true
+  end_of_stream: true
+  stop_buffering_after_segments: 4096
+  max_metadata_length: 4096
+  parse: "iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii"
+  parse: "\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272"
+  parse: "\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272\272"
+  parse: "\244\244\020\007\360\244\017-bin\213\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377c*[)\244(\244-\017\333\360\'\360b\203cin\t!!"
+}

test/core/transport/chttp2/hpack_parser_input_size_corpus/empty

@@ -0,0 +1 @@
+

test/core/transport/chttp2/hpack_parser_input_size_fuzzer.cc

@@ -0,0 +1,151 @@
+// Copyright 2023 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.
+
+// For all inputs, ensure parsing one byte at a time produces the same result as
+// parsing the entire input at once.
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <memory>
+#include <string>
+
+#include "absl/cleanup/cleanup.h"
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
+#include "absl/strings/str_cat.h"
+
+#include <grpc/event_engine/memory_allocator.h>
+#include <grpc/slice.h>
+#include <grpc/support/alloc.h>
+#include <grpc/support/time.h>
+
+#include "src/core/ext/transport/chttp2/transport/hpack_parser.h"
+#include "src/core/lib/gprpp/ref_counted_ptr.h"
+#include "src/core/lib/gprpp/status_helper.h"
+#include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/iomgr/exec_ctx.h"
+#include "src/core/lib/resource_quota/arena.h"
+#include "src/core/lib/resource_quota/memory_quota.h"
+#include "src/core/lib/resource_quota/resource_quota.h"
+#include "src/core/lib/slice/slice.h"
+#include "src/core/lib/transport/metadata_batch.h"
+#include "test/core/util/slice_splitter.h"
+
+bool squelch = true;
+bool leak_check = true;
+
+namespace grpc_core {
+namespace {
+
+class TestEncoder {
+ public:
+  std::string result() { return out_; }
+
+  void Encode(const Slice& key, const Slice& value) {
+    out_.append(
+        absl::StrCat(key.as_string_view(), ": ", value.as_string_view(), "\n"));
+  }
+
+  template <typename T, typename V>
+  void Encode(T, const V& v) {
+    out_.append(absl::StrCat(T::key(), ": ", T::DisplayValue(v), "\n"));
+  }
+
+ private:
+  std::string out_;
+};
+
+bool IsStreamError(const absl::Status& status) {
+  intptr_t stream_id;
+  return grpc_error_get_int(status, StatusIntProperty::kStreamId, &stream_id);
+}
+
+absl::StatusOr<std::string> TestVector(grpc_slice_split_mode mode,
+                                       Slice input) {
+  MemoryAllocator memory_allocator = MemoryAllocator(
+      ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator("test"));
+  auto arena = MakeScopedArena(1024, &memory_allocator);
+  ExecCtx exec_ctx;
+  grpc_slice* slices;
+  size_t nslices;
+  size_t i;
+
+  grpc_metadata_batch b(arena.get());
+
+  HPackParser parser;
+  parser.BeginFrame(
+      &b, 1024, 1024, HPackParser::Boundary::None, HPackParser::Priority::None,
+      HPackParser::LogInfo{1, HPackParser::LogInfo::kHeaders, false});
+
+  grpc_split_slices(mode, const_cast<grpc_slice*>(&input.c_slice()), 1, &slices,
+                    &nslices);
+  auto cleanup_slices = absl::MakeCleanup([slices, nslices] {
+    for (size_t i = 0; i < nslices; i++) {
+      grpc_slice_unref(slices[i]);
+    }
+    gpr_free(slices);
+  });
+
+  absl::Status found_err;
+  for (i = 0; i < nslices; i++) {
+    ExecCtx exec_ctx;
+    auto err = parser.Parse(slices[i], i == nslices - 1);
+    if (!err.ok()) {
+      if (!IsStreamError(err)) return err;
+      if (found_err.ok()) found_err = err;
+    }
+  }
+  if (!found_err.ok()) return found_err;
+
+  TestEncoder encoder;
+  b.Encode(&encoder);
+  return encoder.result();
+}
+
+std::string Stringify(absl::StatusOr<std::string> result) {
+  if (result.ok()) {
+    return absl::StrCat("OK\n", result.value());
+  } else {
+    intptr_t stream_id;
+    bool has_stream = grpc_error_get_int(
+        result.status(), StatusIntProperty::kStreamId, &stream_id);
+    return absl::StrCat(
+        has_stream ? "STREAM" : "CONNECTION", " ERROR: ",
+        result.status().ToString(absl::StatusToStringMode::kWithNoExtraData));
+  }
+}
+
+}  // namespace
+}  // namespace grpc_core
+
+extern gpr_timespec (*gpr_now_impl)(gpr_clock_type clock_type);
+
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
+  gpr_now_impl = [](gpr_clock_type clock_type) {
+    return gpr_timespec{10, 0, clock_type};
+  };
+  auto slice = grpc_core::Slice::FromCopiedBuffer(data, size);
+  auto full = grpc_core::Stringify(
+      grpc_core::TestVector(GRPC_SLICE_SPLIT_IDENTITY, slice.Ref()));
+  auto one_byte = grpc_core::Stringify(
+      grpc_core::TestVector(GRPC_SLICE_SPLIT_ONE_BYTE, slice.Ref()));
+  if (full != one_byte) {
+    fprintf(stderr, "MISMATCHED RESULTS\nFULL SLICE: %s\nONE BYTE: %s\n",
+            full.c_str(), one_byte.c_str());
+    abort();
+  }
+  return 0;
+}

test/core/transport/chttp2/hpack_parser_table_test.cc

@@ -37,7 +37,7 @@ void AssertIndex(const HPackTable* tbl, uint32_t idx, const char* key,
                  const char* value) {
   const auto* md = tbl->Lookup(idx);
   ASSERT_NE(md, nullptr);
-  EXPECT_EQ(md->DebugString(), absl::StrCat(key, ": ", value));
+  EXPECT_EQ(md->md.DebugString(), absl::StrCat(key, ": ", value));
 }
 }  // namespace
 
@@ -119,8 +119,12 @@ TEST(HpackParserTableTest, ManyAdditions) {
     std::string value = absl::StrCat("VALUE.", i);
     auto key_slice = Slice::FromCopiedString(key);
     auto value_slice = Slice::FromCopiedString(value);
-    auto memento =
-        HPackTable::Memento(std::move(key_slice), std::move(value_slice));
+    auto memento = HPackTable::Memento{
+        ParsedMetadata<grpc_metadata_batch>(
+            ParsedMetadata<grpc_metadata_batch>::FromSlicePair{},
+            std::move(key_slice), std::move(value_slice),
+            key.length() + value.length() + 32),
+        absl::OkStatus()};
     auto add_err = tbl.Add(std::move(memento));
     ASSERT_EQ(add_err, absl::OkStatus());
     AssertIndex(&tbl, 1 + hpack_constants::kLastStaticEntry, key.c_str(),

test/core/transport/chttp2/hpack_parser_test.cc

@@ -20,53 +20,64 @@
 
 #include <stdlib.h>
 
-#include <initializer_list>
 #include <memory>
 #include <string>
 
+#include "absl/cleanup/cleanup.h"
 #include "absl/status/status.h"
+#include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
-#include "absl/strings/str_format.h"
+#include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <grpc/event_engine/memory_allocator.h>
 #include <grpc/grpc.h>
 #include <grpc/slice.h>
+#include <grpc/status.h>
 #include <grpc/support/alloc.h>
 
-#include "src/core/lib/gprpp/crash.h"
 #include "src/core/lib/gprpp/ref_counted_ptr.h"
 #include "src/core/lib/gprpp/status_helper.h"
+#include "src/core/lib/gprpp/time.h"
 #include "src/core/lib/iomgr/exec_ctx.h"
 #include "src/core/lib/resource_quota/arena.h"
 #include "src/core/lib/resource_quota/memory_quota.h"
 #include "src/core/lib/resource_quota/resource_quota.h"
 #include "src/core/lib/slice/slice.h"
+#include "src/core/lib/transport/error_utils.h"
 #include "test/core/util/parse_hexstring.h"
 #include "test/core/util/slice_splitter.h"
 #include "test/core/util/test_config.h"
 
+namespace grpc_core {
+namespace {
+
+const uint32_t kFailureIsConnectionError = 1;
+const uint32_t kWithPriority = 2;
+const uint32_t kEndOfStream = 4;
+const uint32_t kEndOfHeaders = 8;
+
 struct TestInput {
-  const char* input;
-  const char* expected_parse;
+  absl::string_view input;
+  absl::StatusOr<absl::string_view> expected_parse;
+  uint32_t flags;
 };
 
 struct Test {
   absl::optional<size_t> table_size;
+  absl::optional<size_t> max_metadata_size;
   std::vector<TestInput> inputs;
 };
 
 class ParseTest : public ::testing::TestWithParam<Test> {
  public:
-  ParseTest() {
-    grpc_init();
-    parser_ = std::make_unique<grpc_core::HPackParser>();
-  }
+  ParseTest() { grpc_init(); }
 
   ~ParseTest() override {
     {
-      grpc_core::ExecCtx exec_ctx;
+      ExecCtx exec_ctx;
       parser_.reset();
     }
 
@@ -74,6 +85,7 @@ class ParseTest : public ::testing::TestWithParam<Test> {
   }
 
   void SetUp() override {
+    parser_ = std::make_unique<HPackParser>();
     if (GetParam().table_size.has_value()) {
       parser_->hpack_table()->SetMaxBytes(GetParam().table_size.value());
       EXPECT_EQ(parser_->hpack_table()->SetCurrentTableSize(
@@ -82,15 +94,21 @@ class ParseTest : public ::testing::TestWithParam<Test> {
     }
   }
 
-  void TestVector(grpc_slice_split_mode mode, const char* hexstring,
-                  std::string expect) {
-    grpc_core::MemoryAllocator memory_allocator =
-        grpc_core::MemoryAllocator(grpc_core::ResourceQuota::Default()
-                                       ->memory_quota()
-                                       ->CreateMemoryAllocator("test"));
-    auto arena = grpc_core::MakeScopedArena(1024, &memory_allocator);
-    grpc_core::ExecCtx exec_ctx;
-    grpc_slice input = parse_hexstring(hexstring);
+  static bool IsStreamError(const absl::Status& status) {
+    intptr_t stream_id;
+    return grpc_error_get_int(status, StatusIntProperty::kStreamId, &stream_id);
+  }
+
+  void TestVector(grpc_slice_split_mode mode,
+                  absl::optional<size_t> max_metadata_size,
+                  absl::string_view hexstring,
+                  absl::StatusOr<absl::string_view> expect, uint32_t flags) {
+    MemoryAllocator memory_allocator = MemoryAllocator(
+        ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator(
+            "test"));
+    auto arena = MakeScopedArena(1024, &memory_allocator);
+    ExecCtx exec_ctx;
+    auto input = ParseHexstring(hexstring);
     grpc_slice* slices;
     size_t nslices;
     size_t i;
@@ -98,32 +116,59 @@ class ParseTest : public ::testing::TestWithParam<Test> {
     grpc_metadata_batch b(arena.get());
 
     parser_->BeginFrame(
-        &b, 4096, 4096, grpc_core::HPackParser::Boundary::None,
-        grpc_core::HPackParser::Priority::None,
-        grpc_core::HPackParser::LogInfo{
-            1, grpc_core::HPackParser::LogInfo::kHeaders, false});
+        &b, max_metadata_size.value_or(4096), max_metadata_size.value_or(4096),
+        (flags & kEndOfStream)
+            ? HPackParser::Boundary::EndOfStream
+            : ((flags & kEndOfHeaders) ? HPackParser::Boundary::EndOfHeaders
+                                       : HPackParser::Boundary::None),
+        flags & kWithPriority ? HPackParser::Priority::Included
+                              : HPackParser::Priority::None,
+        HPackParser::LogInfo{1, HPackParser::LogInfo::kHeaders, false});
 
-    grpc_split_slices(mode, &input, 1, &slices, &nslices);
-    grpc_slice_unref(input);
+    grpc_split_slices(mode, const_cast<grpc_slice*>(&input.c_slice()), 1,
+                      &slices, &nslices);
+    auto cleanup_slices = absl::MakeCleanup([slices, nslices] {
+      for (size_t i = 0; i < nslices; i++) {
+        grpc_slice_unref(slices[i]);
+      }
+      gpr_free(slices);
+    });
 
+    bool saw_error = false;
     for (i = 0; i < nslices; i++) {
-      grpc_core::ExecCtx exec_ctx;
+      ExecCtx exec_ctx;
       auto err = parser_->Parse(slices[i], i == nslices - 1);
-      if (!err.ok()) {
-        grpc_core::Crash(
-            absl::StrFormat("Unexpected parse error: %s",
-                            grpc_core::StatusToString(err).c_str()));
+      if (!err.ok() && (flags & kFailureIsConnectionError) == 0) {
+        EXPECT_TRUE(IsStreamError(err)) << err;
+      }
+      if (!saw_error && !err.ok()) {
+        // one byte at a time mode might fail with a stream error early
+        if (mode == GRPC_SLICE_SPLIT_ONE_BYTE &&
+            (flags & kFailureIsConnectionError) && IsStreamError(err)) {
+          continue;
+        }
+        grpc_status_code code;
+        std::string message;
+        grpc_error_get_status(err, Timestamp::InfFuture(), &code, &message,
+                              nullptr, nullptr);
+        EXPECT_EQ(code, static_cast<grpc_status_code>(expect.status().code()))
+            << err;
+        EXPECT_THAT(message, ::testing::HasSubstr(expect.status().message()))
+            << err;
+        saw_error = true;
+        if (flags & kFailureIsConnectionError) return;
       }
     }
 
-    for (i = 0; i < nslices; i++) {
-      grpc_slice_unref(slices[i]);
+    if (!saw_error) {
+      EXPECT_TRUE(expect.ok()) << expect.status();
     }
-    gpr_free(slices);
 
-    TestEncoder encoder;
-    b.Encode(&encoder);
-    EXPECT_EQ(encoder.result(), expect);
+    if (expect.ok()) {
+      TestEncoder encoder;
+      b.Encode(&encoder);
+      EXPECT_EQ(encoder.result(), *expect) << "Input: " << hexstring;
+    }
   }
 
  private:
@@ -131,7 +176,7 @@ class ParseTest : public ::testing::TestWithParam<Test> {
    public:
     std::string result() { return out_; }
 
-    void Encode(const grpc_core::Slice& key, const grpc_core::Slice& value) {
+    void Encode(const Slice& key, const Slice& value) {
       out_.append(absl::StrCat(key.as_string_view(), ": ",
                                value.as_string_view(), "\n"));
     }
@@ -146,41 +191,45 @@ class ParseTest : public ::testing::TestWithParam<Test> {
     std::string out_;
   };
 
-  std::unique_ptr<grpc_core::HPackParser> parser_;
+  std::unique_ptr<HPackParser> parser_;
 };
 
 TEST_P(ParseTest, WholeSlices) {
   for (const auto& input : GetParam().inputs) {
-    TestVector(GRPC_SLICE_SPLIT_MERGE_ALL, input.input, input.expected_parse);
+    TestVector(GRPC_SLICE_SPLIT_MERGE_ALL, GetParam().max_metadata_size,
+               input.input, input.expected_parse, input.flags);
   }
 }
 
 TEST_P(ParseTest, OneByteAtATime) {
   for (const auto& input : GetParam().inputs) {
-    TestVector(GRPC_SLICE_SPLIT_ONE_BYTE, input.input, input.expected_parse);
+    TestVector(GRPC_SLICE_SPLIT_ONE_BYTE, GetParam().max_metadata_size,
+               input.input, input.expected_parse, input.flags);
   }
 }
 
 INSTANTIATE_TEST_SUITE_P(
     ParseTest, ParseTest,
     ::testing::Values(
-        Test{
-            {},
-            {
-                // D.2.1
-                {"400a 6375 7374 6f6d 2d6b 6579 0d63 7573"
-                 "746f 6d2d 6865 6164 6572",
-                 "custom-key: custom-header\n"},
-                // D.2.2
-                {"040c 2f73 616d 706c 652f 7061 7468", ":path: /sample/path\n"},
-                // D.2.3
-                {"1008 7061 7373 776f 7264 0673 6563 7265"
-                 "74",
-                 "password: secret\n"},
-                // D.2.4
-                {"82", ":method: GET\n"},
-            }},
         Test{{},
+             {},
+             {
+                 // D.2.1
+                 {"400a 6375 7374 6f6d 2d6b 6579 0d63 7573"
+                  "746f 6d2d 6865 6164 6572",
+                  "custom-key: custom-header\n", 0},
+                 // D.2.2
+                 {"040c 2f73 616d 706c 652f 7061 7468", ":path: /sample/path\n",
+                  0},
+                 // D.2.3
+                 {"1008 7061 7373 776f 7264 0673 6563 7265"
+                  "74",
+                  "password: secret\n", 0},
+                 // D.2.4
+                 {"82", ":method: GET\n", 0},
+             }},
+        Test{{},
+             {},
              {
                  // D.3.1
                  {"8286 8441 0f77 7777 2e65 7861 6d70 6c65"
@@ -188,14 +237,16 @@ INSTANTIATE_TEST_SUITE_P(
                   ":path: /\n"
                   ":authority: www.example.com\n"
                   ":method: GET\n"
-                  ":scheme: http\n"},
+                  ":scheme: http\n",
+                  0},
                  // D.3.2
                  {"8286 84be 5808 6e6f 2d63 6163 6865",
                   ":path: /\n"
                   ":authority: www.example.com\n"
                   ":method: GET\n"
                   ":scheme: http\n"
-                  "cache-control: no-cache\n"},
+                  "cache-control: no-cache\n",
+                  0},
                  // D.3.3
                  {"8287 85bf 400a 6375 7374 6f6d 2d6b 6579"
                   "0c63 7573 746f 6d2d 7661 6c75 65",
@@ -203,9 +254,11 @@ INSTANTIATE_TEST_SUITE_P(
                   ":authority: www.example.com\n"
                   ":method: GET\n"
                   ":scheme: https\n"
-                  "custom-key: custom-value\n"},
+                  "custom-key: custom-value\n",
+                  0},
              }},
         Test{{},
+             {},
              {
                  // D.4.1
                  {"8286 8441 8cf1 e3c2 e5f2 3a6b a0ab 90f4"
@@ -213,14 +266,16 @@ INSTANTIATE_TEST_SUITE_P(
                   ":path: /\n"
                   ":authority: www.example.com\n"
                   ":method: GET\n"
-                  ":scheme: http\n"},
+                  ":scheme: http\n",
+                  0},
                  // D.4.2
                  {"8286 84be 5886 a8eb 1064 9cbf",
                   ":path: /\n"
                   ":authority: www.example.com\n"
                   ":method: GET\n"
                   ":scheme: http\n"
-                  "cache-control: no-cache\n"},
+                  "cache-control: no-cache\n",
+                  0},
                  // D.4.3
                  {"8287 85bf 4088 25a8 49e9 5ba9 7d7f 8925"
                   "a849 e95b b8e8 b4bf",
@@ -228,9 +283,11 @@ INSTANTIATE_TEST_SUITE_P(
                   ":authority: www.example.com\n"
                   ":method: GET\n"
                   ":scheme: https\n"
-                  "custom-key: custom-value\n"},
+                  "custom-key: custom-value\n",
+                  0},
              }},
         Test{{256},
+             {},
              {
                  // D.5.1
                  {"4803 3330 3258 0770 7269 7661 7465 611d"
@@ -241,13 +298,15 @@ INSTANTIATE_TEST_SUITE_P(
                   ":status: 302\n"
                   "cache-control: private\n"
                   "date: Mon, 21 Oct 2013 20:13:21 GMT\n"
-                  "location: https://www.example.com\n"},
+                  "location: https://www.example.com\n",
+                  0},
                  // D.5.2
                  {"4803 3330 37c1 c0bf",
                   ":status: 307\n"
                   "cache-control: private\n"
                   "date: Mon, 21 Oct 2013 20:13:21 GMT\n"
-                  "location: https://www.example.com\n"},
+                  "location: https://www.example.com\n",
+                  0},
                  // D.5.3
                  {"88c1 611d 4d6f 6e2c 2032 3120 4f63 7420"
                   "3230 3133 2032 303a 3133 3a32 3220 474d"
@@ -262,9 +321,11 @@ INSTANTIATE_TEST_SUITE_P(
                   "location: https://www.example.com\n"
                   "content-encoding: gzip\n"
                   "set-cookie: foo=ASDJKHQKBZXOQWEOPIUAXQWEOIU; max-age=3600; "
-                  "version=1\n"},
+                  "version=1\n",
+                  0},
              }},
         Test{{256},
+             {},
              {
                  // D.6.1
                  {"4882 6402 5885 aec3 771a 4b61 96d0 7abe"
@@ -274,13 +335,15 @@ INSTANTIATE_TEST_SUITE_P(
                   ":status: 302\n"
                   "cache-control: private\n"
                   "date: Mon, 21 Oct 2013 20:13:21 GMT\n"
-                  "location: https://www.example.com\n"},
+                  "location: https://www.example.com\n",
+                  0},
                  // D.6.2
                  {"4883 640e ffc1 c0bf",
                   ":status: 307\n"
                   "cache-control: private\n"
                   "date: Mon, 21 Oct 2013 20:13:21 GMT\n"
-                  "location: https://www.example.com\n"},
+                  "location: https://www.example.com\n",
+                  0},
                  // D.6.3
                  {"88c1 6196 d07a be94 1054 d444 a820 0595"
                   "040b 8166 e084 a62d 1bff c05a 839b d9ab"
@@ -293,18 +356,356 @@ INSTANTIATE_TEST_SUITE_P(
                   "location: https://www.example.com\n"
                   "content-encoding: gzip\n"
                   "set-cookie: foo=ASDJKHQKBZXOQWEOPIUAXQWEOIU; max-age=3600; "
-                  "version=1\n"},
+                  "version=1\n",
+                  0},
              }},
         Test{{},
+             {1024},
+             {{"3fc43fc4", absl::InternalError("Attempt to make hpack table"),
+               kFailureIsConnectionError}}},
+        Test{{},
+             {},
+             {{"3ba4a41007f0a40f2d62696e8b632a5b29a40fa4a4281007f0",
+               absl::InternalError("Invalid HPACK index received"),
+               kFailureIsConnectionError}}},
+        Test{{},
+             {},
+             {{"2aa41007f0a40f2d62696e8163a41f1f00275bf0692862a4dbf0f00963",
+               absl::InternalError(
+                   "More than two max table size changes in a single frame"),
+               kFailureIsConnectionError}}},
+        Test{{},
+             {},
+             {{"2aa41007f0a40f2d62696e8363271f00275bf06928626e2d213fa40fdbf0212"
+               "8215cf00963",
+               absl::InternalError("illegal base64 encoding"), 0}}},
+        Test{{},
+             {},
+             {{"a4a41007f0a40f2d62696e8b635b29282d2762696e3b0921213fa41fdbf0211"
+               "007f07b282d62696ef009215c0921e51fe91b3b3f47ed5b282821215cf0",
+               absl::InternalError(
+                   "More than two max table size changes in a single frame"),
+               kFailureIsConnectionError}}},
+        Test{
+            {},
+            {},
+            {{"696969696969696969696969696969696969696969696969696969696969696"
+              "969696969696969696969696969696969696969696969696969696969696969"
+              "6969696969696969696969696969bababababababababababababababababab"
+              "abababababababababababababababababababababababababababababababa"
+              "bababababababababababababababababababababababababababababababab"
+              "abababababaa4a41007f0a40f2d62696e8bffffffffffffffffffffffffffff"
+              "ffffffffffff632a5b29a428a42d0fdbf027f0628363696e092121",
+              absl::InternalError("integer overflow in hpack integer decoding"),
+              kEndOfHeaders | kFailureIsConnectionError}}},
+        Test{{},
+             {},
+             {{"0e 00 00 df",
+               absl::InternalError(
+                   "Error parsing ':status' metadata: error=not an integer"),
+               0}}},
+        Test{{},
+             {},
              {
                  // Binary metadata: created using:
                  // tools/codegen/core/gen_header_frame.py
-                 //    --compression inc --no_framing --hex
+                 //    --compression inc --no_framing --output hexstr
                  //    < test/core/transport/chttp2/binary-metadata.headers
                  {"40 09 61 2e 62 2e 63 2d 62 69 6e 0c 62 32 31 6e 4d 6a 41 79 "
                   "4d 51 3d 3d",
-                  "a.b.c-bin: omg2021\n"},
-             }}));
+                  "a.b.c-bin: omg2021\n", 0},
+             }},
+        Test{{},
+             {},
+             {// Binary metadata: created using:
+              // tools/codegen/core/gen_header_frame.py
+              //    --compression inc --no_framing --output hexstr
+              //    < test/core/transport/chttp2/bad-base64.headers
+              {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974"
+               "27732074756573646179",
+               absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+                                   "error=illegal base64 encoding"),
+               0},
+              {"be",
+               absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+                                   "error=illegal base64 encoding"),
+               0}}},
+        Test{{},
+             {},
+             {// created using:
+              // tools/codegen/core/gen_header_frame.py
+              //    --compression inc --no_framing --output hexstr
+              //    < test/core/transport/chttp2/bad-te.headers
+              {"400274650767617262616765",
+               absl::InternalError("Error parsing 'te' metadata"), 0},
+              {"be", absl::InternalError("Error parsing 'te' metadata"), 0}}},
+        Test{{},
+             128,
+             {
+                 {// Generated with: tools/codegen/core/gen_header_frame.py
+                  // --compression inc --output hexstr --no_framing <
+                  // test/core/transport/chttp2/large-metadata.headers
+                  "40096164616c64726964610a6272616e64796275636b40086164616c6772"
+                  "696d04746f6f6b4008616d6172616e74680a6272616e64796275636b4008"
+                  "616e67656c6963610762616767696e73",
+                  absl::ResourceExhaustedError(
+                      "received metadata size exceeds hard limit"),
+                  0},
+                 // Should be able to look up the added elements individually
+                 // (do not corrupt the hpack table test!)
+                 {"be", "angelica: baggins\n", 0},
+                 {"bf", "amaranth: brandybuck\n", 0},
+                 {"c0", "adalgrim: took\n", 0},
+                 {"c1", "adaldrida: brandybuck\n", 0},
+                 // But not as a whole - that exceeds metadata limits for one
+                 // request again
+                 {"bebfc0c1",
+                  absl::ResourceExhaustedError(
+                      "received metadata size exceeds hard limit"),
+                  0},
+             }},
+        Test{
+            {},
+            {},
+            {{"be", absl::InternalError("Invalid HPACK index received"),
+              kFailureIsConnectionError}},
+        },
+        Test{
+            {},
+            {},
+            {{"80", absl::InternalError("Illegal hpack op code"),
+              kFailureIsConnectionError}},
+        },
+        Test{
+            {},
+            {},
+            {{"29", "", kFailureIsConnectionError}},
+        },
+        Test{
+            {},
+            {},
+            {{"", "", kWithPriority}},
+        },
+        Test{
+            {},
+            {},
+            {{"f5", absl::InternalError("Invalid HPACK index received"),
+              kFailureIsConnectionError}},
+        },
+        Test{
+            {},
+            {},
+            {{"0f", "", 0}},
+        },
+        Test{
+            {},
+            {},
+            {{"7f", "", 0}},
+        },
+        Test{
+            {},
+            {},
+            {{"1bffffff7c1b", "", 0}},
+        },
+        Test{
+            {},
+            {},
+            {{"ffffffffff00ff",
+              absl::InternalError("Invalid HPACK index received"),
+              kFailureIsConnectionError}},
+        },
+        Test{
+            {},
+            {},
+            {{"ff8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8"
+              "d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d"
+              "8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8"
+              "d8d8d8d8d8d8d8d",
+              absl::InternalError("integer overflow in hpack integer decoding"),
+              kFailureIsConnectionError}}},
+        Test{
+            {},
+            {9},
+            {{"3f6672616d6573207ba2020656e645f6f665f686561646572733a2074727565a"
+              "2020656e645f6f665f73747265616d3a2074727565a202073746f705f6275666"
+              "66572696e675f61667465725f7365676d656e74733a2039a202070617273653a"
+              "20225c3030305c3030305c3030305c3030305c3030305c3030305c3030305c30"
+              "30305c3030305c3030305c3030305c3030305c3030305c3030305c3030305c30"
+              "30305c3030305c3030305c3030305c3030305c3030305c3030305c3030305c",
+              absl::ResourceExhaustedError(
+                  "received metadata size exceeds hard limit"),
+              kWithPriority}}},
+        Test{{},
+             {},
+             {{"52046772706300073a737461747573033230300e7f",
+               ":status: 200\naccept-ranges: grpc\n", 0}}},
+        Test{{},
+             {},
+             {{"a4a41007f0a40f2d62696e8beda42d5b63272129a410626907",
+               absl::InternalError("illegal base64 encoding"), 0}}},
+        Test{
+            // haiku segment: 149bytes*2, a:a segment: 34 bytes
+            // So we arrange for one less than the total so we force a hpack
+            // table overflow
+            {149 * 2 + 34 - 1},
+            {},
+            {
+                {// Generated with: tools/codegen/core/gen_header_frame.py
+                 // --compression inc --output hexstr --no_framing <
+                 // test/core/transport/chttp2/long-base64.headers
+                 "4005782d62696e70516d467a5a545930494756755932396b6157356e4f67"
+                 "704a644342305957746c6379426961573568636e6b675a47463059534268"
+                 "626d5167625746725a584d6761585167644756346443344b56584e6c5a6e5"
+                 "67349475a766369427a644739796157356e49475a706247567a4c673d3d",
+                 // Haiku by Bard.
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n",
+                 0},
+                // Should go into the hpack table (x-bin: ... is 149 bytes long
+                // by hpack rules)
+                {"be",
+                 "x-bin: Base64 encoding:\nIt takes binary data and "
+                 "makes it text.\nUseful for storing files.\n",
+                 0},
+                // Add another copy
+                {"4005782d62696e70516d467a5a545930494756755932396b6157356e4f67"
+                 "704a644342305957746c6379426961573568636e6b675a47463059534268"
+                 "626d5167625746725a584d6761585167644756346443344b56584e6c5a6e5"
+                 "67349475a766369427a644739796157356e49475a706247567a4c673d3d",
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n",
+                 0},
+                // 149*2 == 298, so we should have two copies in the hpack table
+                {"bebf",
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n"
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n",
+                 0},
+                // Add some very short headers (should push the first long thing
+                // out)
+                // Generated with: tools/codegen/core/gen_header_frame.py
+                // --compression inc --output hexstr --no_framing <
+                // test/core/transport/chttp2/short.headers
+                {"4001610161", "a: a\n", 0},
+                // First two entries should be what was just pushed and then one
+                // long entry
+                {"bebf",
+                 "a: a\nx-bin: Base64 encoding:\nIt takes binary data and "
+                 "makes "
+                 "it text.\nUseful for storing files.\n",
+                 0},
+                // Third entry should be unprobable (it's no longer in the
+                // table!)
+                {"c0", absl::InternalError("Invalid HPACK index received"),
+                 kFailureIsConnectionError},
+            }},
+        Test{
+            // haiku segment: 149bytes*2, a:a segment: 34 bytes
+            // So we arrange for one less than the total so we force a hpack
+            // table overflow
+            {149 * 2 + 34 - 1},
+            {},
+            {
+                {// Generated with: tools/codegen/core/gen_header_frame.py
+                 // --compression inc --output hexstr --no_framing --huff <
+                 // test/core/transport/chttp2/long-base64.headers
+                 "4005782d62696edbd94e1f7fbbf983262e36f313fd47c9bab54d5e592f5d0"
+                 "73e49a09eae987c9b9c95759bf7161073dd7678e9d9347cb0d9fbf9a261fe"
+                 "6c9a4c5c5a92f359b8fe69a3f6ae28c98bf7b90d77dc989ff43e4dd59317e"
+                 "d71e2e3ef3cd041",
+                 // Haiku by Bard.
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n",
+                 0},
+                // Should go into the hpack table (x-bin: ... is 149 bytes long
+                // by hpack rules)
+                {"be",
+                 "x-bin: Base64 encoding:\nIt takes binary data and "
+                 "makes it text.\nUseful for storing files.\n",
+                 0},
+                // Add another copy
+                {"4005782d62696edbd94e1f7fbbf983262e36f313fd47c9bab54d5e592f5d0"
+                 "73e49a09eae987c9b9c95759bf7161073dd7678e9d9347cb0d9fbf9a261fe"
+                 "6c9a4c5c5a92f359b8fe69a3f6ae28c98bf7b90d77dc989ff43e4dd59317e"
+                 "d71e2e3ef3cd041",
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n",
+                 0},
+                // 149*2 == 298, so we should have two copies in the hpack table
+                {"bebf",
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n"
+                 "x-bin: Base64 encoding:\nIt takes binary data and makes it "
+                 "text.\nUseful for storing files.\n",
+                 0},
+                // Add some very short headers (should push the first long thing
+                // out)
+                // Generated with: tools/codegen/core/gen_header_frame.py
+                // --compression inc --output hexstr --no_framing <
+                // test/core/transport/chttp2/short.headers
+                {"4001610161", "a: a\n", 0},
+                // First two entries should be what was just pushed and then one
+                // long entry
+                {"bebf",
+                 "a: a\nx-bin: Base64 encoding:\nIt takes binary data and "
+                 "makes "
+                 "it text.\nUseful for storing files.\n",
+                 0},
+                // Third entry should be unprobable (it's no longer in the
+                // table!)
+                {"c0", absl::InternalError("Invalid HPACK index received"),
+                 kFailureIsConnectionError},
+            }},
+        Test{{}, {}, {{"7a", "", 0}}},
+        Test{{},
+             {},
+             {{"60",
+               absl::InternalError("Incomplete header at the end of a "
+                                   "header/continuation sequence"),
+               kEndOfStream | kFailureIsConnectionError}}},
+        Test{{},
+             {},
+             {{"89", ":status: 204\n", 0},
+              {"89", ":status: 204\n", 0},
+              {"393939393939393939393939393939393939393939",
+               absl::InternalError(
+                   "More than two max table size changes in a single frame"),
+               kFailureIsConnectionError}}},
+        Test{{},
+             {},
+             {{"4005782d62696edbd94e1f7fbbf983267e36a313fd47c9bab54d5e592f5d",
+               "", 0}}},
+        Test{{}, {}, {{"72656672657368", "", 0}}},
+        Test{{}, {}, {{"66e6645f74", "", 0}, {"66645f74", "", 0}}},
+        Test{
+            {},
+            {},
+            {{// Generated with: tools/codegen/core/gen_header_frame.py
+              // --compression inc --output hexstr --no_framing <
+              // test/core/transport/chttp2/MiXeD-CaSe.headers
+              "400a4d695865442d436153651073686f756c64206e6f74207061727365",
+              absl::InternalError("Illegal header key: MiXeD-CaSe"), 0},
+             {// Looking up with hpack indices should work, but also return
+              // error
+              "be", absl::InternalError("Illegal header key: MiXeD-CaSe"), 0}}},
+        Test{
+            {},
+            {},
+            {{"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+              absl::InternalError("integer overflow in hpack integer decoding"),
+              kFailureIsConnectionError}}},
+        Test{{},
+             {},
+             {{"dadadadadadadadadadadadadadadadadadadadadadadadadadadadadadadad"
+               "adadadadadadadadadadadadadadadadadadadadadadadadadadadadadadada"
+               "dadadadadadadadadadadadadadadadadadadadadadadadadadadadadadadad"
+               "adadadadadadadadadadadadadadadadadadada",
+               absl::InternalError("Invalid HPACK index received"),
+               kWithPriority | kFailureIsConnectionError}}}));
+
+}  // namespace
+}  // namespace grpc_core
 
 int main(int argc, char** argv) {
   grpc::testing::TestEnvironment env(&argc, argv);

test/core/transport/chttp2/hpack_sync_corpus/crash-0c85d3a3dad81ec97be1a3079ff93f17c25d9723

@@ -0,0 +1,10 @@
+headers {
+  literal_not_idx {
+    key: "\000\000\000\026"
+  }
+}
+headers {
+  literal_inc_idx {
+    value: "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
+  }
+}

test/core/transport/chttp2/hpack_sync_corpus/crash-212b1a7ccb2034b7f21be3b413c2de51fcacef79

@@ -0,0 +1,14 @@
+headers {
+  literal_not_idx_from_idx {
+  }
+}
+headers {
+  literal_not_idx_from_idx {
+    index: 18688
+  }
+}
+headers {
+  literal_inc_idx {
+    key: "\001\000\000\000\000\000\000\003"
+  }
+}

test/core/transport/chttp2/hpack_sync_corpus/crash-298b34c2c15ac7b7fe8174017265d7e3b3313804

@@ -0,0 +1,12 @@
+headers {
+  literal_inc_idx {
+    value: "\013"
+  }
+}
+headers {
+}
+headers {
+  literal_inc_idx {
+    value: "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
+  }
+}

test/core/transport/chttp2/hpack_sync_corpus/crash-5d27241617bb39dc456910aa4fa18431f2458dc4

@@ -0,0 +1,9 @@
+headers {
+}
+headers {
+}
+headers {
+  literal_inc_idx {
+    key: "E"
+  }
+}

test/core/transport/chttp2/hpack_sync_corpus/crash-85f9f9c7c971ec3fa839df8b14b4bad15d13f4ea

@@ -0,0 +1,24 @@
+use_true_binary_metadata: true
+headers {
+  literal_not_idx {
+    key: "OOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO"
+  }
+}
+headers {
+  literal_inc_idx {
+    key: "grpc-tags-bin"
+    value: "grpc-tags-bin"
+  }
+}
+headers {
+  literal_inc_idx {
+    key: "grpc-tags-bin"
+  }
+}
+headers {
+  literal_inc_idx {
+    key: "grpc-tags-bin"
+  }
+}
+headers {
+}

test/core/transport/chttp2/hpack_sync_fuzzer.cc

@@ -0,0 +1,173 @@
+// Copyright 2023 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 <inttypes.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <algorithm>
+#include <memory>
+#include <string>
+#include <tuple>
+#include <utility>
+#include <vector>
+
+#include "absl/status/status.h"
+#include "absl/strings/escaping.h"
+#include "absl/strings/match.h"
+
+#include <grpc/support/log.h>
+
+#include "src/core/ext/transport/chttp2/transport/hpack_encoder.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_encoder_table.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_parser.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_parser_table.h"
+#include "src/core/lib/gprpp/ref_counted_ptr.h"
+#include "src/core/lib/gprpp/status_helper.h"
+#include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/iomgr/exec_ctx.h"
+#include "src/core/lib/resource_quota/arena.h"
+#include "src/core/lib/resource_quota/memory_quota.h"
+#include "src/core/lib/resource_quota/resource_quota.h"
+#include "src/core/lib/slice/slice.h"
+#include "src/core/lib/slice/slice_buffer.h"
+#include "src/core/lib/transport/metadata_batch.h"
+#include "src/libfuzzer/libfuzzer_macro.h"
+#include "test/core/transport/chttp2/hpack_sync_fuzzer.pb.h"
+
+bool squelch = true;
+bool leak_check = true;
+
+static void dont_log(gpr_log_func_args* /*args*/) {}
+
+namespace grpc_core {
+namespace {
+
+bool IsStreamError(const absl::Status& status) {
+  intptr_t stream_id;
+  return grpc_error_get_int(status, StatusIntProperty::kStreamId, &stream_id);
+}
+
+void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) {
+  // STAGE 1: Encode the fuzzing input into a buffer (encode_output)
+  HPackCompressor compressor;
+  SliceBuffer encode_output;
+  hpack_encoder_detail::Encoder encoder(
+      &compressor, msg.use_true_binary_metadata(), encode_output);
+  for (const auto& header : msg.headers()) {
+    switch (header.ty_case()) {
+      case hpack_sync_fuzzer::Header::TY_NOT_SET:
+        break;
+      case hpack_sync_fuzzer::Header::kIndexed:
+        if (header.indexed() == 0) continue;  // invalid encoding
+        encoder.EmitIndexed(header.indexed());
+        break;
+      case hpack_sync_fuzzer::Header::kLiteralIncIdx:
+        if (header.literal_inc_idx().key().length() +
+                header.literal_inc_idx().value().length() >
+            HPackEncoderTable::MaxEntrySize() / 2) {
+          // Not an interesting case to fuzz
+          continue;
+        }
+        if (absl::EndsWith(header.literal_inc_idx().value(), "-bin")) {
+          std::ignore = encoder.EmitLitHdrWithBinaryStringKeyIncIdx(
+              Slice::FromCopiedString(header.literal_inc_idx().key()),
+              Slice::FromCopiedString(header.literal_inc_idx().value()));
+        } else {
+          std::ignore = encoder.EmitLitHdrWithNonBinaryStringKeyIncIdx(
+              Slice::FromCopiedString(header.literal_inc_idx().key()),
+              Slice::FromCopiedString(header.literal_inc_idx().value()));
+        }
+        break;
+      case hpack_sync_fuzzer::Header::kLiteralNotIdx:
+        if (absl::EndsWith(header.literal_not_idx().value(), "-bin")) {
+          encoder.EmitLitHdrWithBinaryStringKeyNotIdx(
+              Slice::FromCopiedString(header.literal_not_idx().key()),
+              Slice::FromCopiedString(header.literal_not_idx().value()));
+        } else {
+          encoder.EmitLitHdrWithNonBinaryStringKeyNotIdx(
+              Slice::FromCopiedString(header.literal_not_idx().key()),
+              Slice::FromCopiedString(header.literal_not_idx().value()));
+        }
+        break;
+      case hpack_sync_fuzzer::Header::kLiteralNotIdxFromIdx:
+        if (header.literal_not_idx_from_idx().index() == 0) continue;
+        encoder.EmitLitHdrWithBinaryStringKeyNotIdx(
+            header.literal_not_idx_from_idx().index(),
+            Slice::FromCopiedString(header.literal_not_idx_from_idx().value()));
+        break;
+    }
+  }
+
+  // STAGE 2: Decode the buffer (encode_output) into a list of headers
+  HPackParser parser;
+  auto memory_allocator =
+      ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator(
+          "test-allocator");
+  auto arena = MakeScopedArena(1024, &memory_allocator);
+  ExecCtx exec_ctx;
+  grpc_metadata_batch read_metadata(arena.get());
+  parser.BeginFrame(
+      &read_metadata, 1024, 1024, HPackParser::Boundary::EndOfHeaders,
+      HPackParser::Priority::None,
+      HPackParser::LogInfo{1, HPackParser::LogInfo::kHeaders, false});
+  std::vector<std::pair<size_t, absl::Status>> seen_errors;
+  for (size_t i = 0; i < encode_output.Count(); i++) {
+    auto err = parser.Parse(encode_output.c_slice_at(i),
+                            i == (encode_output.Count() - 1));
+    if (!err.ok()) {
+      seen_errors.push_back(std::make_pair(i, err));
+      // If we get a connection error (i.e. not a stream error), stop parsing,
+      // return.
+      if (!IsStreamError(err)) return;
+    }
+  }
+
+  // STAGE 3: If we reached here we either had a stream error or no error
+  // parsing.
+  // Either way, the hpack tables should be of the same size between client and
+  // server.
+  const auto encoder_size = encoder.hpack_table().test_only_table_size();
+  const auto parser_size = parser.hpack_table()->test_only_table_size();
+  const auto encoder_elems = encoder.hpack_table().test_only_table_elems();
+  const auto parser_elems = parser.hpack_table()->num_entries();
+  if (encoder_size != parser_size || encoder_elems != parser_elems) {
+    fprintf(stderr, "Encoder size: %d Parser size: %d\n", encoder_size,
+            parser_size);
+    fprintf(stderr, "Encoder elems: %d Parser elems: %d\n", encoder_elems,
+            parser_elems);
+    if (!seen_errors.empty()) {
+      fprintf(stderr, "Seen errors during parse:\n");
+      for (const auto& error : seen_errors) {
+        fprintf(stderr, "  [slice %" PRIdPTR "] %s\n", error.first,
+                error.second.ToString().c_str());
+      }
+    }
+    fprintf(stderr, "Encoded data:\n");
+    for (size_t i = 0; i < encode_output.Count(); i++) {
+      fprintf(
+          stderr, "  [slice %" PRIdPTR "]: %s\n", i,
+          absl::BytesToHexString(encode_output[i].as_string_view()).c_str());
+    }
+    abort();
+  }
+}
+
+}  // namespace
+}  // namespace grpc_core
+
+DEFINE_PROTO_FUZZER(const hpack_sync_fuzzer::Msg& msg) {
+  if (squelch) gpr_set_log_function(dont_log);
+  grpc_core::FuzzOneInput(msg);
+}

test/core/transport/chttp2/hpack_sync_fuzzer.proto

@@ -0,0 +1,43 @@
+// Copyright 2023 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.
+
+syntax = "proto3";
+
+package hpack_sync_fuzzer;
+
+message Empty {}
+
+message StringKeyValue {
+    string key = 1;
+    string value = 2;
+}
+
+message IndexedKeyValue {
+    uint32 index = 1;
+    string value = 2;
+}
+
+message Header {
+    oneof ty {
+        uint32 indexed = 1;
+        StringKeyValue literal_inc_idx = 2;
+        StringKeyValue literal_not_idx = 3;
+        IndexedKeyValue literal_not_idx_from_idx = 4;
+    }
+}
+
+message Msg {
+  bool use_true_binary_metadata = 1;
+  repeated Header headers = 2;
+}

test/core/transport/chttp2/large-metadata.headers

@@ -0,0 +1,8 @@
+# 19 -> 51
+adaldrida: brandybuck
+# 12 -> 95
+adalgrim: took
+# 18 -> 145
+amaranth: brandybuck
+# 15 -> 192
+angelica: baggins

test/core/transport/chttp2/long-base64.headers

@@ -0,0 +1 @@
+x-bin: QmFzZTY0IGVuY29kaW5nOgpJdCB0YWtlcyBiaW5hcnkgZGF0YSBhbmQgbWFrZXMgaXQgdGV4dC4KVXNlZnVsIGZvciBzdG9yaW5nIGZpbGVzLg==

test/core/transport/chttp2/short.headers

@@ -0,0 +1 @@
+a: a

test/core/transport/parsed_metadata_test.cc

@@ -223,16 +223,17 @@ INSTANTIATE_TYPED_TEST_SUITE_P(My, TraitSpecializedTest, InterestingTraits);
 TEST(KeyValueTest, Simple) {
   using PM = ParsedMetadata<grpc_metadata_batch>;
   using PMPtr = std::unique_ptr<PM>;
-  PMPtr p = std::make_unique<PM>(Slice::FromCopiedString("key"),
-                                 Slice::FromCopiedString("value"));
+  PMPtr p =
+      std::make_unique<PM>(PM::FromSlicePair{}, Slice::FromCopiedString("key"),
+                           Slice::FromCopiedString("value"), 40);
   EXPECT_EQ(p->DebugString(), "key: value");
   EXPECT_EQ(p->transport_size(), 40);
-  PM p2 = p->WithNewValue(Slice::FromCopiedString("some_other_value"),
-                          [](absl::string_view msg, const Slice& value) {
-                            ASSERT_TRUE(false)
-                                << "Should not be called: msg=" << msg
-                                << ", value=" << value.as_string_view();
-                          });
+  PM p2 = p->WithNewValue(
+      Slice::FromCopiedString("some_other_value"), strlen("some_other_value"),
+      [](absl::string_view msg, const Slice& value) {
+        ASSERT_TRUE(false) << "Should not be called: msg=" << msg
+                           << ", value=" << value.as_string_view();
+      });
   EXPECT_EQ(p->DebugString(), "key: value");
   EXPECT_EQ(p2.DebugString(), "key: some_other_value");
   EXPECT_EQ(p2.transport_size(), 51);
@@ -247,18 +248,19 @@ TEST(KeyValueTest, Simple) {
 TEST(KeyValueTest, LongKey) {
   using PM = ParsedMetadata<grpc_metadata_batch>;
   using PMPtr = std::unique_ptr<PM>;
-  PMPtr p = std::make_unique<PM>(Slice::FromCopiedString(std::string(60, 'a')),
-                                 Slice::FromCopiedString("value"));
+  PMPtr p = std::make_unique<PM>(PM::FromSlicePair{},
+                                 Slice::FromCopiedString(std::string(60, 'a')),
+                                 Slice::FromCopiedString("value"), 60 + 5 + 32);
   EXPECT_EQ(
       p->DebugString(),
       "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: value");
   EXPECT_EQ(p->transport_size(), 97);
-  PM p2 = p->WithNewValue(Slice::FromCopiedString("some_other_value"),
-                          [](absl::string_view msg, const Slice& value) {
-                            ASSERT_TRUE(false)
-                                << "Should not be called: msg=" << msg
-                                << ", value=" << value.as_string_view();
-                          });
+  PM p2 = p->WithNewValue(
+      Slice::FromCopiedString("some_other_value"), strlen("some_other_value"),
+      [](absl::string_view msg, const Slice& value) {
+        ASSERT_TRUE(false) << "Should not be called: msg=" << msg
+                           << ", value=" << value.as_string_view();
+      });
   EXPECT_EQ(
       p->DebugString(),
       "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: value");

test/core/util/parse_hexstring.cc

@@ -21,17 +21,18 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <grpc/slice.h>
 #include <grpc/support/log.h>
 
-grpc_slice parse_hexstring(const char* hexstring) {
+namespace grpc_core {
+Slice ParseHexstring(absl::string_view hexstring) {
   size_t nibbles = 0;
-  const char* p = nullptr;
   uint8_t* out;
   uint8_t temp;
   grpc_slice slice;
 
-  for (p = hexstring; *p; p++) {
-    nibbles += (*p >= '0' && *p <= '9') || (*p >= 'a' && *p <= 'f');
+  for (auto c : hexstring) {
+    nibbles += (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f');
   }
 
   GPR_ASSERT((nibbles & 1) == 0);
@@ -41,13 +42,13 @@ grpc_slice parse_hexstring(const char* hexstring) {
 
   nibbles = 0;
   temp = 0;
-  for (p = hexstring; *p; p++) {
-    if (*p >= '0' && *p <= '9') {
-      temp = static_cast<uint8_t>(temp << 4) | static_cast<uint8_t>(*p - '0');
+  for (auto c : hexstring) {
+    if (c >= '0' && c <= '9') {
+      temp = static_cast<uint8_t>(temp << 4) | static_cast<uint8_t>(c - '0');
       nibbles++;
-    } else if (*p >= 'a' && *p <= 'f') {
+    } else if (c >= 'a' && c <= 'f') {
       temp =
-          static_cast<uint8_t>(temp << 4) | static_cast<uint8_t>(*p - 'a' + 10);
+          static_cast<uint8_t>(temp << 4) | static_cast<uint8_t>(c - 'a' + 10);
       nibbles++;
     }
     if (nibbles == 2) {
@@ -56,5 +57,6 @@ grpc_slice parse_hexstring(const char* hexstring) {
     }
   }
 
-  return slice;
+  return Slice(slice);
 }
+}  // namespace grpc_core