Removes the return value of `HpackHeaderTable::TryAddEntry` and updates tests Since the only production caller of `TryAddEntry` ignores the return value, we remove it from the API to allow future storage changes that might invalidate pointers. Tests are updated to not rely on returned pointers. The API does not give callers any visibility into, much less pointer access to, the actual stored `HpackEntry` structures. Protected by removes an unused API surface; not protected. PiperOrigin-RevId: 967992304
diff --git a/quiche/http2/hpack/hpack_encoder_test.cc b/quiche/http2/hpack/hpack_encoder_test.cc index 14e4178..c0aa99f 100644 --- a/quiche/http2/hpack/hpack_encoder_test.cc +++ b/quiche/http2/hpack/hpack_encoder_test.cc
@@ -154,13 +154,13 @@ void SetUp() override { // Populate dynamic entries into the table fixture. For simplicity each // entry has name.size() + value.size() == 10. - key_1_ = peer_.table()->TryAddEntry("key1", "value1"); + peer_.table()->TryAddEntry("key1", "value1"); key_1_index_ = dynamic_table_insertions_++; - key_2_ = peer_.table()->TryAddEntry("key2", "value2"); + peer_.table()->TryAddEntry("key2", "value2"); key_2_index_ = dynamic_table_insertions_++; - cookie_a_ = peer_.table()->TryAddEntry("cookie", "a=bb"); + peer_.table()->TryAddEntry("cookie", "a=bb"); cookie_a_index_ = dynamic_table_insertions_++; - cookie_c_ = peer_.table()->TryAddEntry("cookie", "c=dd"); + peer_.table()->TryAddEntry("cookie", "c=dd"); cookie_c_index_ = dynamic_table_insertions_++; // No further insertions may occur without evictions. @@ -270,10 +270,7 @@ const size_t kInitialDynamicTableSize = 4 * (10 + 32); const HpackEntry* static_; - const HpackEntry* key_1_; - const HpackEntry* key_2_; - const HpackEntry* cookie_a_; - const HpackEntry* cookie_c_; + size_t key_1_index_; size_t key_2_index_; size_t cookie_a_index_; @@ -422,10 +419,9 @@ ExpectIndex(DynamicIndexToWireIndex(key_2_index_)); quiche::HttpHeaderBlock headers; - headers[key_2_->name()] = key_2_->value(); + headers["key2"] = "value2"; CompareWithExpectedEncoding(headers); - EXPECT_THAT(headers_observed_, - ElementsAre(Pair(key_2_->name(), key_2_->value()))); + EXPECT_THAT(headers_observed_, ElementsAre(Pair("key2", "value2"))); } TEST_P(HpackEncoderTest, SingleStaticIndex) { @@ -451,12 +447,12 @@ ExpectIndexedLiteral(DynamicIndexToWireIndex(key_2_index_), "value3"); quiche::HttpHeaderBlock headers; - headers[key_2_->name()] = "value3"; + headers["key2"] = "value3"; CompareWithExpectedEncoding(headers); // A new entry was inserted and added to the reference set. HpackEntry* new_entry = peer_.table_peer().dynamic_entries()->front().get(); - EXPECT_EQ(new_entry->name(), key_2_->name()); + EXPECT_EQ(new_entry->name(), "key2"); EXPECT_EQ(new_entry->value(), "value3"); } @@ -493,7 +489,7 @@ ExpectIndexedLiteral("key3", "value3"); quiche::HttpHeaderBlock headers; - headers[key_1_->name()] = key_1_->value(); + headers["key1"] = "value1"; headers["key3"] = "value3"; CompareWithExpectedEncoding(headers); }
diff --git a/quiche/http2/hpack/hpack_header_table.cc b/quiche/http2/hpack/hpack_header_table.cc index 0fbcc6e..e29ad43 100644 --- a/quiche/http2/hpack/hpack_header_table.cc +++ b/quiche/http2/hpack/hpack_header_table.cc
@@ -136,8 +136,8 @@ } } -const HpackEntry* HpackHeaderTable::TryAddEntry(absl::string_view name, - absl::string_view value) { +void HpackHeaderTable::TryAddEntry(absl::string_view name, + absl::string_view value) { // Since |dynamic_entries_| has iterator stability, |name| and |value| are // valid even after evicting other entries and push_front() making room for // the new one. @@ -148,7 +148,7 @@ // Entire table has been emptied, but there's still insufficient room. QUICHE_DCHECK(dynamic_entries_.empty()); QUICHE_DCHECK_EQ(0u, size_); - return nullptr; + return; } const size_t index = dynamic_table_insertions_; @@ -184,11 +184,8 @@ dynamic_name_index_.insert(std::make_pair(new_entry->name(), index)); QUICHE_CHECK(insert_result.second); } - size_ += entry_size; ++dynamic_table_insertions_; - - return dynamic_entries_.front().get(); } } // namespace spdy
diff --git a/quiche/http2/hpack/hpack_header_table.h b/quiche/http2/hpack/hpack_header_table.h index 24aeaa3..59b04bc 100644 --- a/quiche/http2/hpack/hpack_header_table.h +++ b/quiche/http2/hpack/hpack_header_table.h
@@ -91,8 +91,7 @@ // to be evicted, but they may point to an entry which is not. // The added HpackEntry is returned, or NULL is returned if all entries were // evicted and the empty table is of insufficent size for the representation. - const HpackEntry* TryAddEntry(absl::string_view name, - absl::string_view value); + void TryAddEntry(absl::string_view name, absl::string_view value); private: // Returns number of evictions required to enter |name| & |value|.
diff --git a/quiche/http2/hpack/hpack_header_table_test.cc b/quiche/http2/hpack/hpack_header_table_test.cc index 978b493..71709d8 100644 --- a/quiche/http2/hpack/hpack_header_table_test.cc +++ b/quiche/http2/hpack/hpack_header_table_test.cc
@@ -112,8 +112,9 @@ table_.EvictionSet(it->name(), it->value(), &begin, &end); EXPECT_EQ(0, distance(begin, end)); - const HpackEntry* entry = table_.TryAddEntry(it->name(), it->value()); - EXPECT_NE(entry, static_cast<HpackEntry*>(nullptr)); + size_t old_size = peer_.dynamic_entries().size(); + table_.TryAddEntry(it->name(), it->value()); + EXPECT_EQ(old_size + 1, peer_.dynamic_entries().size()); } } @@ -147,7 +148,8 @@ const HpackEntry* first_static_entry = peer_.GetFirstStaticEntry(); const HpackEntry* last_static_entry = peer_.GetLastStaticEntry(); - const HpackEntry* entry = table_.TryAddEntry("header-key", "Header Value"); + table_.TryAddEntry("header-key", "Header Value"); + const HpackEntry* entry = peer_.dynamic_entries().front().get(); EXPECT_EQ("header-key", entry->name()); EXPECT_EQ("Header Value", entry->value()); @@ -250,17 +252,18 @@ TEST_F(HpackHeaderTableTest, SetSizes) { std::string key = "key", value = "value"; - const HpackEntry* entry1 = table_.TryAddEntry(key, value); - const HpackEntry* entry2 = table_.TryAddEntry(key, value); - const HpackEntry* entry3 = table_.TryAddEntry(key, value); + size_t entry_size = HpackEntry::Size(key, value); + table_.TryAddEntry(key, value); + table_.TryAddEntry(key, value); + table_.TryAddEntry(key, value); // Set exactly large enough. No Evictions. - size_t max_size = entry1->Size() + entry2->Size() + entry3->Size(); + size_t max_size = entry_size * 3; table_.SetMaxSize(max_size); EXPECT_EQ(3u, peer_.dynamic_entries().size()); // Set just too small. One eviction. - max_size = entry1->Size() + entry2->Size() + entry3->Size() - 1; + max_size = entry_size * 3 - 1; table_.SetMaxSize(max_size); EXPECT_EQ(2u, peer_.dynamic_entries().size()); @@ -273,7 +276,7 @@ // SETTINGS_HEADER_TABLE_SIZE upper-bounds |table_.max_size()|, // and will force evictions. - max_size = entry3->Size() - 1; + max_size = entry_size - 1; table_.SetSettingsHeaderTableSize(max_size); EXPECT_EQ(max_size, table_.max_size()); EXPECT_EQ(max_size, table_.settings_size_bound()); @@ -282,30 +285,31 @@ TEST_F(HpackHeaderTableTest, EvictionCountForEntry) { std::string key = "key", value = "value"; - const HpackEntry* entry1 = table_.TryAddEntry(key, value); - const HpackEntry* entry2 = table_.TryAddEntry(key, value); - size_t entry3_size = HpackEntry::Size(key, value); + size_t entry_size = HpackEntry::Size(key, value); + table_.TryAddEntry(key, value); + table_.TryAddEntry(key, value); // Just enough capacity for third entry. - table_.SetMaxSize(entry1->Size() + entry2->Size() + entry3_size); + table_.SetMaxSize(entry_size * 3); EXPECT_EQ(0u, peer_.EvictionCountForEntry(key, value)); EXPECT_EQ(1u, peer_.EvictionCountForEntry(key, value + "x")); // No extra capacity. Third entry would force evictions. - table_.SetMaxSize(entry1->Size() + entry2->Size()); + table_.SetMaxSize(entry_size * 2); EXPECT_EQ(1u, peer_.EvictionCountForEntry(key, value)); EXPECT_EQ(2u, peer_.EvictionCountForEntry(key, value + "x")); } TEST_F(HpackHeaderTableTest, EvictionCountToReclaim) { std::string key = "key", value = "value"; - const HpackEntry* entry1 = table_.TryAddEntry(key, value); - const HpackEntry* entry2 = table_.TryAddEntry(key, value); + size_t entry_size = HpackEntry::Size(key, value); + table_.TryAddEntry(key, value); + table_.TryAddEntry(key, value); EXPECT_EQ(1u, peer_.EvictionCountToReclaim(1)); - EXPECT_EQ(1u, peer_.EvictionCountToReclaim(entry1->Size())); - EXPECT_EQ(2u, peer_.EvictionCountToReclaim(entry1->Size() + 1)); - EXPECT_EQ(2u, peer_.EvictionCountToReclaim(entry1->Size() + entry2->Size())); + EXPECT_EQ(1u, peer_.EvictionCountToReclaim(entry_size)); + EXPECT_EQ(2u, peer_.EvictionCountToReclaim(entry_size + 1)); + EXPECT_EQ(2u, peer_.EvictionCountToReclaim(entry_size * 2)); } // Fill a header table with entries. Make sure the entries are in @@ -385,9 +389,7 @@ EXPECT_EQ(peer_.dynamic_entries().size(), peer_.EvictionSet(long_entry.name(), long_entry.value()).size()); - const HpackEntry* new_entry = - table_.TryAddEntry(long_entry.name(), long_entry.value()); - EXPECT_EQ(new_entry, static_cast<HpackEntry*>(nullptr)); + table_.TryAddEntry(long_entry.name(), long_entry.value()); EXPECT_EQ(0u, peer_.dynamic_entries().size()); }