phongn commented on PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#issuecomment-5608597981
Two corrections to things I said earlier in this PR, both from a closer read
after your guard comment. I've rewritten the description accordingly.
**`ram_cache.compress` is not fixed for the life of the process.** I'd
claimed the shared-to-private refresh was unreachable because the record is
`RECU_RESTART_TS`. That marking is advisory: `cache_config_ram_cache_compress`
is linked via `RecEstablishStaticConfigInt32`, and `RecExecConfigUpdateCbs`
fires every pending callback regardless of update type -- the type is only
returned so `traffic_ctl` can report that a restart is needed. So a reload
changes the value live. That makes the refresh reachable (a reload landing
between two racing misses). One thing that surfaced from that and that I've
deliberately left out of this PR: LRU runs its seen filter before the
resident-entry lookup, so it can decline exactly that refresh (CLFUS gates its
filter on new entries only). That's a pre-existing LRU behavior rather than
part of the copy contract, so it will come as its own PR. It also means a 1→0
reload leaves LRU/S3-FIFO holding marshaled private copies that `handleRead` no
longer
routes through the unmarshal -- CLFUS has always had that exposure, this PR
extends it, and the right fix (decide from the buffer's state, not the config)
is in CacheVC. Called out in the description as a follow-up.
**The failure mode was overstated.** The description said the double
unmarshal asserts. `HTTPInfo::unmarshal` returns early on an
already-unmarshaled heap ("must be a ram cache"), so under default config the
shared-buffer mutation is masked. The observable failure is with
`enable_checksum` on, where the recheck in `handleReadDone` walks the mutated
bytes and marks the doc corrupt on every RAM hit after the first. The contract
violation is real either way and the fix stands; the severity framing did not.
Also in the two new commits: the CLFUS resident re-put now refreshes
`e->len` alongside `e->data` (defensive, matching the other two),
`copy_data_in` allocates through `IOBufferData::alloc` instead of setting
`_mem_type` by hand, and the test keys now spread across hash buckets -- they
all landed in bucket 0 before. Two pre-existing items surfaced that I've left
for separate PRs: `doc->len` isn't bounded against the read size before the
put, and CLFUS's decompress-on-hit path charges `compressed_len` for an entry
that now holds `len` bytes.
--
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]
