Copilot commented on code in PR #13611:
URL: https://github.com/apache/trafficserver/pull/13611#discussion_r4010383747
##########
include/proxy/hdrs/URL.h:
##########
@@ -488,6 +496,22 @@ URL::hash_get92(CryptoHash *hash, bool ignore_query,
cache_generation_t generati
url_CryptoHash_get_92(m_url_impl, hash, ignore_query, generation);
}
+/*-------------------------------------------------------------------------
+ -------------------------------------------------------------------------*/
+
+inline bool
+URLImpl::has_path_params() const noexcept
+{
+ return m_ptr_path != nullptr && memchr(m_ptr_path, ';', m_len_path) !=
nullptr;
+}
Review Comment:
`memchr` is used in an inline header method, but this header doesn’t (in
this diff) show an explicit include that guarantees `memchr` is declared. This
can cause build failures in translation units that include `URL.h` without a
prior C string header. Add an explicit `#include <cstring>` (or equivalent) in
`include/proxy/hdrs/URL.h` so the declaration is always available.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -2724,6 +2724,22 @@ HttpTransact::HandleCacheOpenReadHitFreshness(State *s)
s->cache_lookup_result = HttpTransact::CacheLookupResult_t::HIT_STALE;
}
+ // An object found under the 9.2 key cannot be revalidated: the write that
+ // would apply a 304 is a create on the current key, not an update of the
+ // legacy vector, so the cache discards it. Treat a stale one as a miss. The
+ // full response then lands under the current key through the ordinary miss
+ // path and the legacy copy ages out. Only reads migrate; the methods that
+ // invalidate take the delete path, which reaches both keys.
+ if (s->cache_lookup_result == HttpTransact::CacheLookupResult_t::HIT_STALE
&& s->state_machine != nullptr &&
+ CompatCacheKey::is_legacy(s->state_machine->compatibility_cache_lookup)
&&
+ (s->method == HTTP_WKSIDX_GET || s->method == HTTP_WKSIDX_HEAD)) {
+ TxnDbg(dbg_ctl_http_seq, "Stale under the compatibility key, treating as a
miss");
+ s->cache_info.object_read = nullptr;
+ s->cache_lookup_result =
HttpTransact::CacheLookupResult_t::MISS;
+ s->cache_lookup_complete_deferred = false;
+ TRANSACT_RETURN(StateMachineAction_t::API_CACHE_LOOKUP_COMPLETE,
HttpTransact::HandleCacheOpenReadMiss);
+ }
Review Comment:
This converts a completed open_read (HIT_STALE) into a MISS by nulling
`cache_info.object_read`, but it doesn’t ensure the underlying cache read
VC/doc is closed. That risks leaking the cache read handle and/or leaving a doc
busy/locked longer than intended. A concrete way to fix this is to make the
state machine explicitly close the cache read (e.g., `cache_sm.close_read()`)
when taking this compat-stale-to-miss path, rather than only clearing the
pointer in `HttpTransact`.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -5336,11 +5357,48 @@ HttpSM::do_cache_delete_all_alts()
// Do not delete a non-existent object.
ink_assert(t_state.cache_info.object_read);
- SMDbg(dbg_ctl_http_seq, "Issuing cache delete for %s",
t_state.cache_info.lookup_url->string_get_ref());
+ // Address the object that was looked up. A redirect follow can look up a
+ // different URL than cache_info.lookup_url, which is set once and does not
+ // track the redirect when the pristine host header is maintained.
+ URL *url = cache_lookup_url();
+
+ SMDbg(dbg_ctl_http_seq, "Issuing cache delete for %s",
url->string_get_ref());
HttpCacheKey key;
- Cache::generate_key(&key, t_state.cache_info.lookup_url,
t_state.txn_conf->cache_ignore_query,
- t_state.txn_conf->cache_generation_number);
+ Cache::generate_key(&key, url, t_state.txn_conf->cache_ignore_query,
t_state.txn_conf->cache_generation_number);
+ cacheProcessor.remove(nullptr, &key);
+
+ // A migration leaves the legacy copy in place, so the object can live under
+ // both keys. Removing only one of them would let the other be served after
+ // the purge.
+ if (t_state.http_config_param->cache_try_compat_key_read) {
Review Comment:
When the request path already contains a `;` (i.e., `url->has_path_params()`
is true), the legacy (9.2) key and canonical key are byte-identical per the new
hashing behavior. In that case, calling `do_cache_delete_compat_alts()` will
issue a redundant second `remove()` for the same key. Consider short-circuiting
compat deletes when `cache_lookup_url()->has_path_params()` is true (ideally
inside `do_cache_delete_compat_alts()`), mirroring the earlier miss logic that
avoids a redundant compat lookup for these URLs.
##########
src/proxy/hdrs/unit_tests/test_URL.cc:
##########
@@ -852,6 +853,106 @@ TEST_CASE("UrlPathGet", "[url][path_get]")
}
}
+// ATS 9.2 hashed "path" ";" "params" as separate cache-key components, always
+// emitting the separator. The params component was removed from the parser, so
+// ";params" now lives inside the path and the separator is already there. That
+// makes the 9.2 key expressible through the current algorithm: it is the
+// current key of the same URL with exactly one ";" between path and query.
+namespace
+{
+/// A failing REQUIRE unwinds out of these helpers, so the heap is released by
+/// scope exit rather than by a call that the unwind would skip.
+struct HdrHeapDeleter {
+ void
+ operator()(HdrHeap *heap) const
+ {
+ heap->destroy();
+ }
+};
+
+using HdrHeapPtr = std::unique_ptr<HdrHeap, HdrHeapDeleter>;
+
+CryptoHash
+hash92(char const *text)
+{
+ HdrHeapPtr heap{new_HdrHeap()};
+ URL url;
+
+ url.create(heap.get());
+ REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE);
Review Comment:
This test code uses `strlen()` but this file (in the shown include list)
doesn’t include a header that guarantees `strlen` is declared. Add `#include
<cstring>` (or `<string.h>`) to avoid build failures depending on transitive
includes.
--
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]
