This is an automated email from the ASF dual-hosted git repository.
moonchen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 116e663960 Cache: fix cached-header HdrHeap growth on repeated 304
revalidation (#13405)
116e663960 is described below
commit 116e66396042d865d3f513f4fad259614e6afbb3
Author: Mo Chen <[email protected]>
AuthorDate: Mon Jul 20 10:31:54 2026 -0500
Cache: fix cached-header HdrHeap growth on repeated 304 revalidation
(#13405)
A hot, frequently-revalidated cached object grows its cached response
header's HdrHeap without bound until marshal_length crosses the aggregation
fragment limit (AGG_SIZE), after which the cache write can never commit and
the object stays frozen stale.
merge_and_update_headers_for_cache_update unconditionally deleted
Age/ETag/Expires before re-merging. A deleted MIMEField slot is not
reclaimed
until its whole field block empties, so each revalidation stranded a dead
slot.
Delete a caching header only when the 304 omits it; when the 304 carries it,
let the merge overwrite the field in place.
merge_response_header_with_cached_header used a sticky dups_seen flag that,
once any response field had a duplicate, forced every later single-valued
field
down a field_create append path. Reconcile each field name independently,
overwriting cached slots in place and trimming surplus values, so the merge
is
idempotent.
Add regression tests asserting bounded cached-header heap growth across
repeated merges (including the caller's conditional-delete sequence over
cooked
caching headers), and a hidden [.merge-bench] microbenchmark comparing the
new
merge against the previous implementation.
---
src/proxy/http/HttpTransact.cc | 104 ++++++-------
src/proxy/http/unit_tests/test_HttpTransact.cc | 203 +++++++++++++++++++++++++
2 files changed, 255 insertions(+), 52 deletions(-)
diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc
index 7fe4caaaf5..cea93328fb 100644
--- a/src/proxy/http/HttpTransact.cc
+++ b/src/proxy/http/HttpTransact.cc
@@ -5035,11 +5035,23 @@
HttpTransact::merge_and_update_headers_for_cache_update(State *s)
// 10.3.5), but RFC 7232) is clear that the 304 and 200 responses
// must be identical (see section 4.1). This code attempts to strike
// a balance between the two.
- cached_hdr->field_delete(static_cast<std::string_view>(MIME_FIELD_AGE));
- cached_hdr->field_delete(static_cast<std::string_view>(MIME_FIELD_ETAG));
- cached_hdr->field_delete(static_cast<std::string_view>(MIME_FIELD_EXPIRES));
+ //
+ // Only delete a caching header that the server response does NOT carry.
+ // If the response carries it, the merge below overwrites the cached field
+ // in place; deleting it first would strand a dead MIMEField slot on every
+ // revalidation (see merge_response_header_with_cached_header).
+ HTTPHdr *server_response = &s->hdr_info.server_response;
+ if (!server_response->presence(MIME_PRESENCE_AGE)) {
+ cached_hdr->field_delete(static_cast<std::string_view>(MIME_FIELD_AGE));
+ }
+ if (!server_response->presence(MIME_PRESENCE_ETAG)) {
+ cached_hdr->field_delete(static_cast<std::string_view>(MIME_FIELD_ETAG));
+ }
+ if (!server_response->presence(MIME_PRESENCE_EXPIRES)) {
+
cached_hdr->field_delete(static_cast<std::string_view>(MIME_FIELD_EXPIRES));
+ }
- merge_response_header_with_cached_header(cached_hdr,
&s->hdr_info.server_response);
+ merge_response_header_with_cached_header(cached_hdr, server_response);
// Some special processing for 304
if (s->hdr_info.server_response.status_get() == HTTPStatus::NOT_MODIFIED) {
@@ -5217,9 +5229,6 @@ HttpTransact::set_headers_for_cache_write(State *s,
HTTPInfo *cache_info, HTTPHd
void
HttpTransact::merge_response_header_with_cached_header(HTTPHdr *cached_header,
HTTPHdr *response_header)
{
- MIMEField *new_field;
- bool dups_seen = false;
-
for (auto spot = response_header->begin(), limit = response_header->end();
spot != limit; ++spot) {
MIMEField &field{*spot};
auto name{field.name_get()};
@@ -5258,54 +5267,45 @@
HttpTransact::merge_response_header_with_cached_header(HTTPHdr *cached_header, H
if (name.data() == MIME_FIELD_WARNING.c_str()) {
continue;
}
- // Copy all remaining headers with replacement
-
- // Duplicate header fields cause a bug problem
- // since we need to duplicate with replacement.
- // Without dups, we can just nuke what is already
- // there in the cached header. With dups, we
- // can't do this because what is already there
- // may be a dup we've already copied in. If
- // dups show up we look through the remaining
- // header fields in the new response, nuke
- // them in the cached response and then add in
- // the remaining fields one by one from the
- // response header
+
+ // Reconcile the cached header's fields of this name to exactly match the
+ // response's values for this name. Each response field name is processed
+ // once, when we reach its dup-list head (dups are chained in slot order,
so
+ // the head is encountered first). Response values are matched positionally
+ // against the cached fields of the same name: existing cached slots are
+ // overwritten in place, any extra response values are appended, and any
+ // surplus cached values are removed.
//
- if (field.m_next_dup) {
- if (dups_seen == false) {
- // use a second iterator to delete the
- // remaining response headers in the cached response,
- // so that they will be added in the next iterations.
- for (auto spot2 = spot; spot2 != limit; ++spot2) {
- MIMEField &field2{*spot2};
- auto name2{field2.name_get()};
-
- // It is specified above that content type should not
- // be altered here however when a duplicate header
- // is present, all headers following are delete and
- // re-added back. This includes content type if it follows
- // any duplicate header. This leads to the loss of
- // content type in the client response.
- // This ensures that it is not altered when duplicate
- // headers are present.
- if (name2.data() == MIME_FIELD_CONTENT_TYPE.c_str()) {
- continue;
- }
- cached_header->field_delete(name2);
- }
- dups_seen = true;
+ // Reusing cached slots in place keeps the merge idempotent: re-merging an
+ // unchanged response mutates the existing MIMEFields instead of stranding
+ // dead slots. A deleted MIMEField slot is not reclaimed until its whole
+ // field block empties, so a delete-and-recreate merge grows the cached
+ // header heap on every revalidation until it crosses the aggregation
+ // fragment limit and the cache write can no longer commit, freezing the
+ // object stale.
+ if (!field.is_dup_head()) {
+ continue; // a non-head dup: already handled when its dup-list head was
processed
+ }
+
+ MIMEField *cached_field = cached_header->field_find(name);
+ for (MIMEField *resp_field = &field; resp_field != nullptr; resp_field =
resp_field->m_next_dup) {
+ auto value{resp_field->value_get()};
+ if (cached_field != nullptr) {
+ // overwrite an existing cached value of this name in place (reuses
the slot)
+ cached_header->field_value_set(cached_field, value);
+ cached_field = cached_field->m_next_dup;
+ } else {
+ // the response has more values of this name than the cached header
+ MIMEField *new_field = cached_header->field_create(name);
+ cached_header->field_attach(new_field);
+ cached_header->field_value_set(new_field, value);
}
}
-
- auto value{field.value_get()};
-
- if (dups_seen == false) {
- cached_header->value_set(name, value);
- } else {
- new_field = cached_header->field_create(name);
- cached_header->field_attach(new_field);
- cached_header->field_value_set(new_field, value);
+ // remove any surplus cached values of this name the response no longer
carries
+ while (cached_field != nullptr) {
+ MIMEField *next = cached_field->m_next_dup;
+ cached_header->field_delete(cached_field, false /* this dup only */);
+ cached_field = next;
}
}
diff --git a/src/proxy/http/unit_tests/test_HttpTransact.cc
b/src/proxy/http/unit_tests/test_HttpTransact.cc
index fdad3ae46e..58f82e1d4e 100644
--- a/src/proxy/http/unit_tests/test_HttpTransact.cc
+++ b/src/proxy/http/unit_tests/test_HttpTransact.cc
@@ -22,6 +22,8 @@
*/
#include <string_view>
+#include <vector>
+#include <cstdio>
using namespace std::string_view_literals;
@@ -540,5 +542,206 @@ TEST_CASE("HttpTransact", "[http]")
CHECK(field->has_dups() == false);
///////////////////////////////////////
}
+
+ // Regression: repeated 304 revalidation must not grow the cached header
+ // without bound (see merge_response_header_with_cached_header). The
+ // yardstick is HdrHeap::marshal_length() -- the size that gates the cache
+ // write -- NOT fields_count(): dead MIMEField slots grow the heap even
+ // while the live field count stays constant.
+ SECTION("Repeated revalidation does not grow the cached header heap")
+ {
+ struct header {
+ std::string_view name;
+ std::string_view value;
+ };
+
+ auto build = [](HTTPHdr &h, const std::vector<header> &fields) {
+ h.create(HTTPType::RESPONSE);
+ for (auto &&e : fields) {
+ MIMEField *f = h.field_create(e.name);
+ h.field_attach(f);
+ h.field_value_set(f, e.value);
+ }
+ };
+
+ // Merge `response` into a copy of `cached0` `iterations` times and
return
+ // the growth in the cached header's marshalled heap size. A correct
merge
+ // is idempotent, so the growth must be bounded (independent of
iterations).
+ auto merge_growth = [&](const std::vector<header> &cached0, const
std::vector<header> &response, int iterations) -> int {
+ HTTPHdr cached;
+ HTTPHdr resp;
+ ts::PostScript cached_defer([&]() -> void { cached.destroy(); });
+ ts::PostScript resp_defer([&]() -> void { resp.destroy(); });
+ build(cached, cached0);
+ build(resp, response);
+ // One warm-up merge so any one-time restructuring settles before
measuring.
+ HttpTransact::merge_response_header_with_cached_header(&cached, &resp);
+ int const before = cached.m_heap->marshal_length();
+ for (int i = 0; i < iterations; ++i) {
+ HttpTransact::merge_response_header_with_cached_header(&cached,
&resp);
+ }
+ return cached.m_heap->marshal_length() - before;
+ };
+
+ // No duplicated fields: pure in-place replacement, bounded.
+ CHECK(merge_growth(
+ {
+ {"Date", "d0"},
+ {"Expires", "e0"},
+ {"Etag", "t0"}
+ },
+ {{"Date", "d1"}, {"Expires", "e1"}, {"Etag", "t1"}}, 4000) <=
2048);
+
+ // A duplicated field (e.g. Cache-Control: max-age=60, public) ahead of
+ // single-valued fields must not force those single-valued fields down an
+ // appending path. Before the fix the sticky "dups_seen" flag did exactly
+ // that and this grew ~132 bytes per merge (megabytes over a day).
+ CHECK(merge_growth(
+ {
+ {"Cache-Control", "max-age=60"},
+ {"Cache-Control", "public" },
+ {"Expires", "e0" },
+ {"Etag", "t0" }
+ },
+ {{"Cache-Control", "max-age=60"}, {"Cache-Control", "public"},
{"Expires", "e0"}, {"Etag", "t0"}}, 4000) <= 2048);
+
+ // Response carries a duplicated field the cached copy does not yet have.
+ CHECK(merge_growth(
+ {
+ {"Date", "d0"},
+ {"Expires", "e0"},
+ {"Etag", "t0"}
+ },
+ {{"Date", "d0"}, {"Vary", "A"}, {"Vary", "B"}, {"Expires",
"e0"}, {"Etag", "t0"}}, 4000) <= 2048);
+
+ // A response value that changes length every merge (as Expires does on a
+ // real 304) must still be bounded: value_set frees the old string (self-
+ // coalesced) and reuses the slot.
+ {
+ HTTPHdr cached;
+ HTTPHdr resp;
+ ts::PostScript cached_defer([&]() -> void { cached.destroy(); });
+ ts::PostScript resp_defer([&]() -> void { resp.destroy(); });
+ build(cached, {
+ {"Server", "ATS" },
+ {"Cache-Control", "max-age=60"},
+ {"Date", "d0" },
+ {"Expires", "e0" },
+ {"Etag", "t0" }
+ });
+ resp.create(HTTPType::RESPONSE);
+ MIMEField *rf = resp.field_create("Expires"sv);
+ resp.field_attach(rf);
+ resp.field_value_set(rf, "e0"sv);
+ HttpTransact::merge_response_header_with_cached_header(&cached, &resp);
+ int const before = cached.m_heap->marshal_length();
+ char buf[64];
+ for (int i = 0; i < 2000; ++i) {
+ int n = std::snprintf(buf, sizeof(buf), "Sat, 18 Jul 2026
05:%02d:%02d GMT-%d", i % 60, (i * 7) % 60, i % 1000);
+ resp.field_value_set(rf, std::string_view(buf, n));
+ HttpTransact::merge_response_header_with_cached_header(&cached,
&resp);
+ }
+ MIMEField *exp = cached.field_find("Expires"sv);
+ REQUIRE(exp != nullptr);
+ CHECK(exp->has_dups() == false);
+ CHECK(cached.m_heap->marshal_length() - before <= 4096);
+ }
+
+ // Correctness: merging a response that DROPS one of two duplicate values
+ // must leave the cached header with exactly the response's values.
+ {
+ HTTPHdr cached;
+ HTTPHdr resp;
+ ts::PostScript cached_defer([&]() -> void { cached.destroy(); });
+ ts::PostScript resp_defer([&]() -> void { resp.destroy(); });
+ build(cached, {
+ {"Vary", "A" },
+ {"Vary", "B" },
+ {"Vary", "C" },
+ {"Etag", "t0"}
+ });
+ build(resp, {
+ {"Vary", "X" },
+ {"Vary", "Y" },
+ {"Etag", "t1"}
+ });
+ HttpTransact::merge_response_header_with_cached_header(&cached, &resp);
+ int count = 0;
+ std::string_view v0, v1;
+ for (MIMEField *d = cached.field_find("Vary"sv); d != nullptr; d =
d->m_next_dup) {
+ if (count == 0) {
+ v0 = d->value_get();
+ } else if (count == 1) {
+ v1 = d->value_get();
+ }
+ ++count;
+ }
+ CHECK(count == 2); // surplus cached "C" removed
+ CHECK(v0 == "X"sv);
+ CHECK(v1 == "Y"sv);
+ MIMEField *etag = cached.field_find("Etag"sv);
+ REQUIRE(etag != nullptr);
+ CHECK(etag->value_get() == "t1"sv);
+ CHECK(etag->has_dups() == false);
+ }
+
+ // The production caller sequence:
merge_and_update_headers_for_cache_update
+ // deletes a caching header only when the 304 OMITS it, then merges. This
+ // exercises the conditional-delete + the cooked WKS headers
+ // (Cache-Control/Expires/Age) end to end, over many revalidations with a
+ // changing Expires. The cached header heap and the cooked freshness
values
+ // must stay correct and bounded.
+ {
+ HTTPHdr cached;
+ ts::PostScript cached_defer([&]() -> void { cached.destroy(); });
+ build(cached, {
+ {"Server", "ATS" },
+ {"Cache-Control", "max-age=60"},
+ {"Date", "d0" },
+ {"Expires", "e0" },
+ {"Etag", "t0" }
+ });
+
+ int settled = 0;
+ for (int i = 0; i < 2000; ++i) {
+ HTTPHdr resp;
+ ts::PostScript resp_defer([&]() -> void { resp.destroy(); });
+ resp.create(HTTPType::RESPONSE);
+ MIMEField *f = resp.field_create("Cache-Control"sv);
+ resp.field_attach(f);
+ resp.field_value_set(f, "max-age=60"sv);
+ char buf[64];
+ int n = std::snprintf(buf, sizeof(buf), "Sat, 18 Jul 2026
05:%02d:%02d GMT", i % 60, (i * 7) % 60);
+ f = resp.field_create("Expires"sv);
+ resp.field_attach(f);
+ resp.field_value_set(f, std::string_view(buf, n));
+
+ // Mirror merge_and_update_headers_for_cache_update: delete a caching
+ // header only if the response omits it, then merge.
+ if (!resp.presence(MIME_PRESENCE_AGE)) {
+ cached.field_delete(static_cast<std::string_view>(MIME_FIELD_AGE));
+ }
+ if (!resp.presence(MIME_PRESENCE_ETAG)) {
+
cached.field_delete(static_cast<std::string_view>(MIME_FIELD_ETAG));
+ }
+ if (!resp.presence(MIME_PRESENCE_EXPIRES)) {
+
cached.field_delete(static_cast<std::string_view>(MIME_FIELD_EXPIRES));
+ }
+ HttpTransact::merge_response_header_with_cached_header(&cached,
&resp);
+ if (i == 0) {
+ settled = cached.m_heap->marshal_length();
+ }
+ }
+ // Etag was omitted by every 304, so it must have been dropped from
cache.
+ CHECK(cached.field_find("Etag"sv) == nullptr);
+ // Expires stays single-valued and the cooked max-age is intact.
+ MIMEField *exp = cached.field_find("Expires"sv);
+ REQUIRE(exp != nullptr);
+ CHECK(exp->has_dups() == false);
+ CHECK(cached.get_cooked_cc_max_age() == 60);
+ // Heap stays bounded across 2000 revalidations.
+ CHECK(cached.m_heap->marshal_length() - settled <= 4096);
+ }
+ }
}
}
