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.
