From 2dc2c0840687b9c7676ae38ed860496690ad6d64 Mon Sep 17 00:00:00 2001 From: Miles Libbey Date: Wed, 3 Jun 2026 21:00:35 -0700 Subject: [PATCH] prefetch: move cache key acquisition to CACHE_LOOKUP_COMPLETE The plugin read TSHttpTxnCacheLookupUrlGet at POST_REMAP, but the core does not initialize the cache lookup URL until HttpTransact::HandleRequest runs after POST_REMAP returns. The call therefore failed unless a plugin like cachekey was called earlier and populated the URL via TSHttpTxnCacheLookupUrlSet, making prefetch silently dependent on cachekey's presence and remap ordering. Acquire the cache key at CACHE_LOOKUP_COMPLETE instead, which is the first hook where the lookup URL is guaranteed to be set. On transactions where ATS skips cache lookup (non-cacheable methods, internal redirects, cache disabled), the front-end first-pass admission acquire no longer fires. TXN_CLOSE release is gated on _fetchable, so acquire/release symmetry is preserved. Also fix a missing release in the second-pass acquire/uniqueAcquire sequence: if uniqueAcquire fails after acquire succeeds, release the acquired cache key to avoid orphaning it in the policy LRU. The four existing prefetch autests all pair cachekey.so ahead of prefetch.so and would have passed with or without this fix. Adds prefetch_no_cachekey.test.py to regression-cover the standalone case. --- doc/admin-guide/plugins/prefetch.en.rst | 9 +++ plugins/prefetch/plugin.cc | 30 ++++---- .../prefetch/prefetch_no_cachekey.test.py | 76 +++++++++++++++++++ 3 files changed, 98 insertions(+), 17 deletions(-) create mode 100644 tests/gold_tests/pluginTest/prefetch/prefetch_no_cachekey.test.py diff --git a/doc/admin-guide/plugins/prefetch.en.rst b/doc/admin-guide/plugins/prefetch.en.rst index 4f6bf7a7595..7328349dd17 100644 --- a/doc/admin-guide/plugins/prefetch.en.rst +++ b/doc/admin-guide/plugins/prefetch.en.rst @@ -34,6 +34,15 @@ On every **incoming** URL request, the plugin can decide to pre-fetch the **next object** or more objects based on the common URL path pattern and a pre-defined pre-fetch policy. +.. note:: + + The plugin only prefetches on requests that go through a cache lookup. + On transactions where ATS skips the cache lookup -- non-cacheable + methods, or caching turned off (:ts:cv:`proxy.config.http.cache.http` + set to ``0``) -- the plugin does not prefetch. Issuing prefetches into + a cache that will not be consulted would do real work for no cache + benefit. + Currently, most HLS video urls follow a predictable pattern, with most URLs containing a segment number. Since the segments are ~10s of content, the normal usage pattern is to fetch the incremental segment every few seconds. The CDN diff --git a/plugins/prefetch/plugin.cc b/plugins/prefetch/plugin.cc index b935e7928db..cf037802f50 100644 --- a/plugins/prefetch/plugin.cc +++ b/plugins/prefetch/plugin.cc @@ -507,7 +507,6 @@ contHandleFetch(const TSCont contp, TSEvent event, void *edata) // For these cases we need to access the client request switch (event) { - case TS_EVENT_HTTP_POST_REMAP: case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: case TS_EVENT_HTTP_SEND_RESPONSE_HDR: if (TS_SUCCESS != TSHttpTxnClientReqGet(txnp, &reqBuffer, &reqHdrLoc)) { @@ -521,15 +520,16 @@ contHandleFetch(const TSCont contp, TSEvent event, void *edata) } switch (event) { - case TS_EVENT_HTTP_POST_REMAP: { - /* Use the cache key since this has better lookup behavior when using plugins like the cachekey plugin, - * for example multiple URIs can match a single cache key */ + case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: { + /* Use the cache key (multiple URIs can map to one key). CACHE_LOOKUP_COMPLETE is + * the earliest hook where TSHttpTxnCacheLookupUrlGet returns a populated URL. */ if (data->frontend() && data->secondPass()) { /* Create a separate cache key name space to be used only for front-end and second-pass fetch policy checks. */ data->_cachekey.assign("/prefetch"); } if (!appendCacheKey(txnp, reqBuffer, data->_cachekey)) { PrefetchError("failed to get the cache key"); + TSHandleMLocRelease(reqBuffer, TS_NULL_MLOC, reqHdrLoc); TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR); return 0; } @@ -542,17 +542,15 @@ contHandleFetch(const TSCont contp, TSEvent event, void *edata) data->_fetchable = state->acquire(data->_cachekey); PrefetchDebug("request is %s fetchable", data->_fetchable ? " " : " not "); } - } - } - } break; - - case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: { - if (data->frontend()) { - /* front-end instance */ - if (data->secondPass()) { + } else { /* second-pass */ - data->_fetchable = state->acquire(data->_cachekey); - data->_fetchable = data->_fetchable && state->uniqueAcquire(data->_cachekey); + if (state->acquire(data->_cachekey)) { + if (state->uniqueAcquire(data->_cachekey)) { + data->_fetchable = true; + } else { + state->release(data->_cachekey); + } + } PrefetchDebug("request is %s fetchable", data->_fetchable ? " " : " not "); if (isFetchable(txnp, data)) { @@ -734,8 +732,7 @@ contHandleFetch(const TSCont contp, TSEvent event, void *edata) } /* Release the request MLoc */ - if (event == TS_EVENT_HTTP_POST_REMAP || event == TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE || - event == TS_EVENT_HTTP_SEND_RESPONSE_HDR) { + if (event == TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE || event == TS_EVENT_HTTP_SEND_RESPONSE_HDR) { TSHandleMLocRelease(reqBuffer, TS_NULL_MLOC, reqHdrLoc); } @@ -868,7 +865,6 @@ TSRemapDoRemap(void *instance, TSHttpTxn txnp, TSRemapRequestInfo *rri) TSCont cont = TSContCreate(contHandleFetch, TSMutexCreate()); TSContDataSet(cont, static_cast(data)); - TSHttpTxnHookAdd(txnp, TS_HTTP_POST_REMAP_HOOK, cont); TSHttpTxnHookAdd(txnp, TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, cont); TSHttpTxnHookAdd(txnp, TS_HTTP_SEND_RESPONSE_HDR_HOOK, cont); TSHttpTxnHookAdd(txnp, TS_HTTP_TXN_CLOSE_HOOK, cont); diff --git a/tests/gold_tests/pluginTest/prefetch/prefetch_no_cachekey.test.py b/tests/gold_tests/pluginTest/prefetch/prefetch_no_cachekey.test.py new file mode 100644 index 00000000000..c12e27607b5 --- /dev/null +++ b/tests/gold_tests/pluginTest/prefetch/prefetch_no_cachekey.test.py @@ -0,0 +1,76 @@ +''' +''' +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Test prefetch.so plugin without cachekey.so loaded ahead of it. Exercises the +path where TSHttpTxnCacheLookupUrlGet must be served by the core's own cache +lookup URL initialization rather than a prior plugin's TSHttpTxnCacheLookupUrlSet. +''' + +server = Test.MakeOriginServer("server") +for i in list(range(1, 1 + 3)): + request_header = { + "headers": + f"GET /texts/demo-{i}.txt HTTP/1.1\r\n" + "Host: does.not.matter\r\n" # But cannot be omitted. + "\r\n", + "timestamp": "1469733493.993", + "body": "" + } + response_header = { + "headers": "HTTP/1.1 200 OK\r\n" + "Connection: close\r\n" + "Cache-control: max-age=85000\r\n" + "\r\n", + "timestamp": "1469733493.993", + "body": f"This is the body for demo-{i}.txt.\n" + } + server.addResponse("sessionlog.json", request_header, response_header) + +dns = Test.MakeDNServer("dns") + +ts = Test.MakeATSProcess("ts") +ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http|dns|prefetch', + 'proxy.config.dns.nameservers': f"127.0.0.1:{dns.Variables.Port}", + 'proxy.config.dns.resolv_conf': "NULL", + }) +ts.Disk.remap_config.AddLine( + f"map http://domain.in http://127.0.0.1:{server.Variables.Port}" + " @plugin=prefetch.so" + " @pparam=--front=true" + + " @pparam=--fetch-policy=simple" + r" @pparam=--fetch-path-pattern=/(.*-)(\d+)(.*)/$1{$2+1}$3/" + " @pparam=--fetch-count=3") +ts.ReturnCode = Any(0, -2) + +tr = Test.AddTestRun() +tr.Processes.Default.StartBefore(server) +tr.Processes.Default.StartBefore(dns) +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Command = 'echo start TS, HTTP server and DNS.' +tr.Processes.Default.ReturnCode = 0 + +tr = Test.AddTestRun() +tr.MakeCurlCommand(f'--verbose --proxy 127.0.0.1:{ts.Variables.port} http://domain.in/texts/demo-1.txt', ts=ts) +tr.Processes.Default.ReturnCode = 0 + +Test.AddAwaitFileContainsTestRun('Await transactions to finish logging.', ts.Disk.traffic_out.Name, 'demo-4.txt') + +tr = Test.AddTestRun() +tr.Processes.Default.Command = (f"grep 'GET http://' {ts.Disk.traffic_out.Name} | grep -v '127.0.0.1'") +tr.Streams.stdout = "prefetch_simple.gold" +tr.Processes.Default.ReturnCode = 0