Do not evict unacknowledged QPACK entries. See https://crbug.com/1441880. Protected by FLAGS_quic_reloadable_flag_quic_do_not_evict_unacked_entry. PiperOrigin-RevId: 529092490
diff --git a/quiche/quic/core/qpack/qpack_encoder.cc b/quiche/quic/core/qpack/qpack_encoder.cc index 95e55b4..7ac9519 100644 --- a/quiche/quic/core/qpack/qpack_encoder.cc +++ b/quiche/quic/core/qpack/qpack_encoder.cc
@@ -13,6 +13,8 @@ #include "quiche/quic/core/qpack/qpack_instruction_encoder.h" #include "quiche/quic/core/qpack/qpack_required_insert_count.h" #include "quiche/quic/core/qpack/value_splitting_header_list.h" +#include "quiche/quic/platform/api/quic_flag_utils.h" +#include "quiche/quic/platform/api/quic_flags.h" #include "quiche/quic/platform/api/quic_logging.h" namespace quic { @@ -85,13 +87,23 @@ Representations representations; representations.reserve(header_list.size()); - // The index of the oldest entry that must not be evicted. - uint64_t smallest_blocking_index = - blocking_manager_.smallest_blocking_index(); // Entries with index larger than or equal to |known_received_count| are // blocking. const uint64_t known_received_count = blocking_manager_.known_received_count(); + + // The index of the oldest entry that must not be evicted. Blocking entries + // must not be evicted. + uint64_t smallest_non_evictable_index = + blocking_manager_.smallest_blocking_index(); + // Additionally, unacknowledged entries must not be evicted, even if they have + // no outstanding references (see https://crbug.com/1441880 for more context). + if (GetQuicReloadableFlag(quic_do_not_evict_unacked_entry)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_do_not_evict_unacked_entry); + smallest_non_evictable_index = + std::min(smallest_non_evictable_index, known_received_count); + } + // Only entries with index greater than or equal to |draining_index| are // allowed to be referenced. const uint64_t draining_index = @@ -133,7 +145,8 @@ } else { representations.push_back( EncodeIndexedHeaderField(is_static, index, referred_indices)); - smallest_blocking_index = std::min(smallest_blocking_index, index); + smallest_non_evictable_index = + std::min(smallest_non_evictable_index, index); header_table_.set_dynamic_table_entry_referenced(); break; @@ -144,7 +157,7 @@ blocked_stream_limit_exhausted = true; } else if (QpackEntry::Size(name, value) > header_table_.MaxInsertSizeWithoutEvictingGivenEntry( - std::min(smallest_blocking_index, index))) { + std::min(smallest_non_evictable_index, index))) { dynamic_table_insertion_blocked = true; } else { if (can_write_to_encoder_stream) { @@ -155,8 +168,8 @@ uint64_t new_index = header_table_.InsertEntry(name, value); representations.push_back(EncodeIndexedHeaderField( is_static, new_index, referred_indices)); - smallest_blocking_index = - std::min(smallest_blocking_index, index); + smallest_non_evictable_index = + std::min(smallest_non_evictable_index, index); header_table_.set_dynamic_table_entry_referenced(); break; @@ -178,7 +191,7 @@ if (blocking_allowed && QpackEntry::Size(name, value) <= header_table_.MaxInsertSizeWithoutEvictingGivenEntry( - smallest_blocking_index)) { + smallest_non_evictable_index)) { // If allowed, insert entry into dynamic table and refer to it. if (can_write_to_encoder_stream) { encoder_stream_sender_.SendInsertWithNameReference(is_static, @@ -186,8 +199,8 @@ uint64_t new_index = header_table_.InsertEntry(name, value); representations.push_back(EncodeIndexedHeaderField( /* is_static = */ false, new_index, referred_indices)); - smallest_blocking_index = - std::min<uint64_t>(smallest_blocking_index, new_index); + smallest_non_evictable_index = + std::min<uint64_t>(smallest_non_evictable_index, new_index); break; } @@ -204,7 +217,7 @@ blocked_stream_limit_exhausted = true; } else if (QpackEntry::Size(name, value) > header_table_.MaxInsertSizeWithoutEvictingGivenEntry( - std::min(smallest_blocking_index, index))) { + std::min(smallest_non_evictable_index, index))) { dynamic_table_insertion_blocked = true; } else { // If allowed, insert entry with name reference and refer to it. @@ -217,7 +230,8 @@ uint64_t new_index = header_table_.InsertEntry(name, value); representations.push_back(EncodeIndexedHeaderField( is_static, new_index, referred_indices)); - smallest_blocking_index = std::min(smallest_blocking_index, index); + smallest_non_evictable_index = + std::min(smallest_non_evictable_index, index); header_table_.set_dynamic_table_entry_referenced(); break; @@ -229,7 +243,8 @@ // If allowed, refer to entry name directly, with literal value. representations.push_back(EncodeLiteralHeaderFieldWithNameReference( is_static, index, value, referred_indices)); - smallest_blocking_index = std::min(smallest_blocking_index, index); + smallest_non_evictable_index = + std::min(smallest_non_evictable_index, index); header_table_.set_dynamic_table_entry_referenced(); break; @@ -250,7 +265,7 @@ blocked_stream_limit_exhausted = true; } else if (QpackEntry::Size(name, value) > header_table_.MaxInsertSizeWithoutEvictingGivenEntry( - smallest_blocking_index)) { + smallest_non_evictable_index)) { dynamic_table_insertion_blocked = true; } else { if (can_write_to_encoder_stream) { @@ -258,8 +273,8 @@ uint64_t new_index = header_table_.InsertEntry(name, value); representations.push_back(EncodeIndexedHeaderField( /* is_static = */ false, new_index, referred_indices)); - smallest_blocking_index = - std::min<uint64_t>(smallest_blocking_index, new_index); + smallest_non_evictable_index = + std::min<uint64_t>(smallest_non_evictable_index, new_index); break; } @@ -268,10 +283,7 @@ // Encode entry as string literals. // TODO(b/112770235): Consider also adding to dynamic table to improve // compression ratio for subsequent header blocks with peers that do not - // allow any blocked streams. However, RFC 9204 Section 2.1.1 forbids - // evicting an unacknowledged entry even if it has no outstanding - // references, so `smallest_blocking_index` would need to be updated to - // consider that. + // allow any blocked streams. representations.push_back(EncodeLiteralHeaderField(name, value)); break;
diff --git a/quiche/quic/core/qpack/qpack_encoder_test.cc b/quiche/quic/core/qpack/qpack_encoder_test.cc index aa40a8a..0e59048 100644 --- a/quiche/quic/core/qpack/qpack_encoder_test.cc +++ b/quiche/quic/core/qpack/qpack_encoder_test.cc
@@ -10,6 +10,7 @@ #include "absl/strings/escaping.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "quiche/quic/platform/api/quic_flags.h" #include "quiche/quic/platform/api/quic_test.h" #include "quiche/quic/test_tools/qpack/qpack_encoder_peer.h" #include "quiche/quic/test_tools/qpack/qpack_test_utils.h" @@ -628,6 +629,81 @@ EXPECT_EQ(0u, encoder_stream_sent_byte_count_); } +// Regression test for https://crbug.com/1441880. +TEST_F(QpackEncoderTest, UnackedEntryCannotBeEvicted) { + EXPECT_CALL(encoder_stream_sender_delegate_, NumBytesBuffered()) + .WillRepeatedly(Return(0)); + encoder_.SetMaximumBlockedStreams(2); + // With 32 byte overhead per entry, only one entry fits in the dynamic table. + encoder_.SetMaximumDynamicTableCapacity(40); + encoder_.SetDynamicTableCapacity(40); + + QpackEncoderHeaderTable* header_table = + QpackEncoderPeer::header_table(&encoder_); + EXPECT_EQ(0u, header_table->inserted_entry_count()); + EXPECT_EQ(0u, header_table->dropped_entry_count()); + + spdy::Http2HeaderBlock header_list1; + header_list1["foo"] = "bar"; + + // Set Dynamic Table Capacity instruction. + std::string set_dyanamic_table_capacity = absl::HexStringToBytes("3f09"); + // Insert one entry into the dynamic table. + std::string insert_entries1 = absl::HexStringToBytes( + "62" // insert without name reference + "94e7" // Huffman-encoded name "foo" + "03626172"); // value "bar" + EXPECT_CALL(encoder_stream_sender_delegate_, + WriteStreamData(Eq( + absl::StrCat(set_dyanamic_table_capacity, insert_entries1)))); + + EXPECT_EQ( + absl::HexStringToBytes("0200" // prefix + "80"), // dynamic entry with relative index 0 + encoder_.EncodeHeaderList(/* stream_id = */ 1, header_list1, + &encoder_stream_sent_byte_count_)); + + EXPECT_EQ(1u, header_table->inserted_entry_count()); + EXPECT_EQ(0u, header_table->dropped_entry_count()); + + encoder_.OnStreamCancellation(/* stream_id = */ 1); + + // At this point, entry 0 has no references to it, because stream 1 is + // cancelled. However, this entry is unacknowledged, therefore it must not be + // evicted according to RFC 9204 Section 2.1.1. + + spdy::Http2HeaderBlock header_list2; + header_list2["bar"] = "baz"; + + if (GetQuicReloadableFlag(quic_do_not_evict_unacked_entry)) { + EXPECT_EQ(absl::HexStringToBytes("0000" // prefix + "23626172" // literal name "bar" + "0362617a"), // literal value "baz" + encoder_.EncodeHeaderList(/* stream_id = */ 2, header_list2, + &encoder_stream_sent_byte_count_)); + + EXPECT_EQ(1u, header_table->inserted_entry_count()); + EXPECT_EQ(0u, header_table->dropped_entry_count()); + } else { + // Insert one entry into the dynamic table, evicting the first entry. + std::string insert_entries2 = absl::HexStringToBytes( + "43" // insert without name reference + "626172" // name "bar" + "0362617a"); // value "baz" + EXPECT_CALL(encoder_stream_sender_delegate_, + WriteStreamData(Eq(insert_entries2))); + + EXPECT_EQ( + absl::HexStringToBytes("0100" // prefix + "80"), // dynamic entry with relative index 0 + encoder_.EncodeHeaderList(/* stream_id = */ 2, header_list2, + &encoder_stream_sent_byte_count_)); + + EXPECT_EQ(2u, header_table->inserted_entry_count()); + EXPECT_EQ(1u, header_table->dropped_entry_count()); + } +} + } // namespace } // namespace test } // namespace quic
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index 75d63ad..5023083 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -67,6 +67,8 @@ QUIC_FLAG(quic_restart_flag_quic_enable_sending_bandwidth_estimate_when_network_idle_v2, true) // If true, set burst token to 2 in cwnd bootstrapping experiment. QUIC_FLAG(quic_reloadable_flag_quic_conservative_bursts, false) +// If true, unacked QPACK entries with no references will not be evicted. +QUIC_FLAG(quic_reloadable_flag_quic_do_not_evict_unacked_entry, false) // If true, use BBRv2 as the default congestion controller. Takes precedence over --quic_default_to_bbr. QUIC_FLAG(quic_reloadable_flag_quic_default_to_bbr_v2, false) // If true, use a LRU cache to record client addresses of packets received on server\'s original address.