moonchen opened a new pull request, #13405:
URL: https://github.com/apache/trafficserver/pull/13405
# Cache: fix cached-header HdrHeap growth on repeated 304 revalidation
## Problem
A hot, frequently-revalidated cached object slowly grows the `HdrHeap` of its
**cached response header** without bound. Once the header's marshalled size
crosses the aggregation-buffer fragment limit (`AGG_SIZE`, 4 MiB), the cache
write of the updated header can never be admitted again
(`Stripe::add_writer` rejects `agg_len > AGG_SIZE` → `AIO_SOFT_FAILURE` →
`openWriteCloseHeadDone` bails before `dir_overwrite`). The stored object is
then **frozen**: its headers never update, it is always stale, and every
request
re-runs the doomed revalidation write + serve-stale path.
The growth is invisible to `fields_count()` — the *live* field count stays
constant. It shows up only in `HdrHeap::marshal_length()` (the value that
gates
the cache write): dead `MIMEField`/field-block objects accumulate in the
bump-allocated object area.
## Root cause
Two independent contributors, both in the 304 cache-update path:
1. **`merge_and_update_headers_for_cache_update`** unconditionally deletes
`Age`/`ETag`/`Expires` from the cached header before re-merging the 304's
values. A deleted `MIMEField` slot is only reclaimed when its entire
16-slot
block becomes empty, and `HdrHeap::allocate_obj` never reuses an EMPTY
slot —
it only bump-allocates. So each revalidation of an object whose 304
carries a
changing `Expires` strands one dead slot. `HdrHeap::marshal` is a raw
region
`memcpy` (it does not compact), so the dead slots are persisted to disk,
read
back, and re-marshalled larger on the next revalidation — unbounded
growth.
2. **`merge_response_header_with_cached_header`** used a sticky `dups_seen`
flag:
once *any* response field had a duplicate, every *later* single-valued
field
took the `field_create`/`field_attach` **append** path instead of an
in-place
`value_set`, stranding a slot per merge for those fields too.
## Fix
1. In `merge_and_update_headers_for_cache_update`, delete a caching header
only
when the server response **omits** it (guarded on `presence()`). When the
304
carries it, the merge overwrites the cached field in place — no delete +
recreate, so no stranded slot. Final header state is unchanged.
2. Rewrite `merge_response_header_with_cached_header` to reconcile each field
name **independently** at its dup-list head: overwrite existing cached
values
in place (reusing the slot via `field_value_set`), append only genuine
extra
values, and delete any surplus cached values. This makes the merge
idempotent
— re-merging an unchanged response mutates existing fields instead of
stranding dead ones — and removes the sticky-`dups_seen` behavior. It also
preserves the documented `Warning`/`Content-Type` handling.
## Tests
`test_HttpTransact.cc` gains a "Repeated revalidation does not grow the
cached
header heap" section asserting `HdrHeap::marshal_length()` stays bounded
across
up to 4000 merges for: no-dup, leading-dup, response-only-dup, and
changing-length-value cases; plus a correctness case (dropping a duplicate
value
leaves exactly the response's values) and an end-to-end case that mirrors the
caller's conditional-delete sequence over the cooked WKS caching headers
(`Cache-Control`/`Expires`/`Age`), checking the cooked `max-age` and bounded
heap. All existing merge assertions are unchanged.
Verified locally: full `test_http` passes under `dev`, `dev-asan`
(ASAN+UBSAN, no errors), and `release`.
## Performance
Because the merge no longer deletes-then-recreates (and the caller skips
unnecessary deletes), the hot path is **faster**, not slower. A hidden
`[.merge-bench]` microbenchmark times the new merge against a copy of the
previous implementation head-to-head in one process, on a realistic ~14-field
cached 200 + a 304 (release build, ns/op, best of 5, 3 runs):
```
merge only:
typical 304 (no dup): ~ -2% to -8% (in-place replace, ~neutral)
dup 304 (Cache-Ctrl): ~ -27% to -29% (no more append+delete churn)
full revalidation unit (caller delete + merge):
typical 304 (no dup): ~ -18% to -21% (caller skips redundant deletes)
dup 304 (Cache-Ctrl): ~ -26% to -28%
```
## Compatibility / risk
- The resulting cached header field set is equivalent to the previous code
for
realistic 304/200 responses (names, values, dup count, dup value order).
Only
the physical ordering of *distinct* field names can differ when appending
extra dup values, which is not significant (RFC 7230 §3.2.2) and no cached-
response consumer depends on it.
- This prevents the bloat from forming. It does **not** shrink or unfreeze an
object that is *already* bloated on disk: an in-place update cannot reclaim
existing dead object-area bytes (only a fresh full store / copy does).
Objects
already wedged need to leave the cache (purge, full 200 re-store, or
eviction).
## Notes for reviewers
- The `[.merge-bench]` benchmark keeps a copy of the pre-fix merge
(`merge_OLD`)
purely for the A/B timing. It is hidden (`.`-tagged, not run by default).
Happy
to drop it if you'd prefer the PR carry only the regression tests.
- A broader hardening, not done here: give the `HdrHeap` object area a
compaction valve analogous to the existing string-heap `coalesce_str_heaps`
(which already exists precisely to bound "infinite build up of dead
strings on
header merge", see the `INKqa08287` comment in `HdrHeap::allocate_str`).
The
object area has no such valve today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
