prefetch: move cache key acquisition to CACHE_LOOKUP_COMPLETE

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

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<void *>(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);

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