diff --git a/fuzztest/core/transport/chttp2/BUILD b/fuzztest/core/transport/chttp2/BUILD index 84405836a4c..b95ff53e774 100644 --- a/fuzztest/core/transport/chttp2/BUILD +++ b/fuzztest/core/transport/chttp2/BUILD @@ -24,17 +24,3 @@ grpc_fuzz_test( ], deps = ["//src/core:write_size_policy"], ) - -grpc_fuzz_test( - name = "hpack_encoder_timeout_test", - srcs = ["hpack_encoder_timeout_test.cc"], - external_deps = [ - "fuzztest", - "fuzztest_main", - "gtest", - ], - deps = [ - "//:hpack_encoder", - "//:hpack_parser", - ], -) diff --git a/fuzztest/core/transport/chttp2/hpack_encoder_timeout_test.cc b/fuzztest/core/transport/chttp2/hpack_encoder_timeout_test.cc deleted file mode 100644 index 37a686c8b7b..00000000000 --- a/fuzztest/core/transport/chttp2/hpack_encoder_timeout_test.cc +++ /dev/null @@ -1,79 +0,0 @@ -// 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. - -// Test to verify Fuzztest integration - -#include -#include -#include -#include -#include -#include - -#include "absl/random/random.h" -#include "fuzztest/fuzztest.h" -#include "gtest/gtest.h" -#include "src/core/ext/transport/chttp2/transport/hpack_encoder.h" -#include "src/core/ext/transport/chttp2/transport/hpack_parser.h" -#include "src/core/lib/resource_quota/memory_quota.h" -#include "src/core/lib/resource_quota/resource_quota.h" -#include "src/core/lib/gprpp/ref_counted_ptr.h" -#include "src/core/lib/gprpp/time.h" -#include "src/core/lib/resource_quota/arena.h" -#include "src/core/lib/slice/slice_buffer.h" -#include "src/core/lib/transport/metadata_batch.h" - -namespace grpc_core { - -void EncodeTimeouts(std::vector timeouts) { - absl::BitGen bitgen; - ScopedTimeCache time_cache; - time_cache.TestOnlySetNow(Timestamp::ProcessEpoch()); - hpack_encoder_detail::TimeoutCompressorImpl timeout_compressor; - HPackCompressor compressor; - HPackParser parser; - MemoryAllocator memory_allocator = MemoryAllocator( - ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator("test")); - auto arena = MakeScopedArena(1024, &memory_allocator); - for (size_t i = 0; i < timeouts.size(); i++) { - SliceBuffer encoded; - hpack_encoder_detail::Encoder encoder(&compressor, false, encoded); - timeout_compressor.EncodeWith( - "grpc-timeout", - Timestamp::ProcessEpoch() + Duration::Milliseconds(timeouts[i]), - &encoder); - grpc_metadata_batch b(arena.get()); - const uint32_t kMetadataSizeLimit = 3u * 1024 * 1024 * 1024; - parser.BeginFrame( - &b, kMetadataSizeLimit, kMetadataSizeLimit, HPackParser::Boundary::None, - HPackParser::Priority::None, - HPackParser::LogInfo{1, HPackParser::LogInfo::kHeaders, false}); - for (size_t j = 0; j < encoded.Count(); j++) { - EXPECT_TRUE(parser - .Parse(encoded.c_slice_at(j), j == encoded.Count() - 1, - bitgen, nullptr) - .ok()); - } - auto parsed = b.get(GrpcTimeoutMetadata()); - ASSERT_TRUE(parsed.has_value()); - EXPECT_GE(*parsed, - Timestamp::ProcessEpoch() + Duration::Milliseconds(timeouts[i])); - EXPECT_LE(*parsed, Timestamp::ProcessEpoch() + - Duration::Milliseconds(timeouts[i]) * 1.05 + - Duration::Milliseconds(1)); - } -} -FUZZ_TEST(MyTestSuite, EncodeTimeouts); - -} // namespace grpc_core diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index ed615d5acd6..69f2c92dbc6 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc @@ -475,25 +475,31 @@ void Encoder::EncodeRepeatingSliceValue(const absl::string_view& key, void TimeoutCompressorImpl::EncodeWith(absl::string_view key, Timestamp deadline, Encoder* encoder) { - const Timeout timeout = Timeout::FromDuration(deadline - Timestamp::Now()); + Timeout timeout = Timeout::FromDuration(deadline - Timestamp::Now()); auto& table = encoder->hpack_table(); - for (size_t i = 0; i < kNumPreviousValues; i++) { - const auto& previous = previous_timeouts_[i]; - if (!table.ConvertableToDynamicIndex(previous.index)) continue; - const double ratio = timeout.RatioVersus(previous.timeout); + 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) { - encoder->EmitIndexed(table.DynamicIndex(previous.index)); + if (ratio > -3 && ratio <= 0 && + 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, *previous_timeouts_.begin()); return; } } + // Clean out some expired timeouts. + while (!previous_timeouts_.empty() && + !table.ConvertableToDynamicIndex(previous_timeouts_.back().index)) { + previous_timeouts_.pop_back(); + } Slice encoded = timeout.Encode(); uint32_t index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx( Slice::FromStaticString(key), std::move(encoded)); - uint32_t i = next_previous_value_; - ++next_previous_value_; - previous_timeouts_[i % kNumPreviousValues] = PreviousTimeout{timeout, index}; + previous_timeouts_.push_back(PreviousTimeout{timeout, index}); } Encoder::Encoder(HPackCompressor* compressor, bool use_true_binary_metadata, diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.h b/src/core/ext/transport/chttp2/transport/hpack_encoder.h index 500126932e1..12e2df3eb6c 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.h +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.h @@ -276,10 +276,8 @@ class Compressor { }; struct PreviousTimeout { - Timeout timeout = Timeout::FromDuration(Duration::Zero()); - // Dynamic table index of a previously sent timeout - // 0 is guaranteed not in the dynamic table so is a safe initializer - uint32_t index = 0; + Timeout timeout; + uint32_t index; }; class TimeoutCompressorImpl { @@ -287,9 +285,7 @@ class TimeoutCompressorImpl { void EncodeWith(absl::string_view key, Timestamp deadline, Encoder* encoder); private: - static constexpr const size_t kNumPreviousValues = 5; - PreviousTimeout previous_timeouts_[kNumPreviousValues]; - uint32_t next_previous_value_ = 0; + std::vector previous_timeouts_; }; template diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h b/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h index 212ee1b1277..623e50bf620 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h @@ -59,7 +59,6 @@ class HPackEncoderTable { table_elems_ - index; } // Check if an element index is convertable to a dynamic index - // Note that 0 is always not convertable bool ConvertableToDynamicIndex(uint32_t index) const { return index > tail_remote_index_; } diff --git a/src/core/lib/transport/timeout_encoding.h b/src/core/lib/transport/timeout_encoding.h index 6e0eab972f0..3b011fcb487 100644 --- a/src/core/lib/transport/timeout_encoding.h +++ b/src/core/lib/transport/timeout_encoding.h @@ -61,8 +61,8 @@ class Timeout { static Timeout FromMinutes(int64_t minutes); static Timeout FromHours(int64_t hours); - uint16_t value_ = 0; - Unit unit_ = Unit::kNanoseconds; + uint16_t value_; + Unit unit_; }; absl::optional ParseTimeout(const Slice& text);