From b7998f9a92605f57c091795439253c9aa1598ebf Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Mon, 31 Jan 2022 19:20:19 +0000 Subject: [PATCH] Revert fixes for #8539 Fixing #8539 is easy to do in a common case, but it causing issues in corner cases. For now I'm going to revert the fixes for this and circle back around later to see whether this can be solved in a different way. This reverts the following commits: Revert "Commenting TSHttpTxnCacheLookupStatusGet need_to_revalidate (#8621)" This reverts commit 4678ae34321002c584bd40ff6a1e4a128dae8bb1 Revert "TSHttpTxnCacheLookupStatusGet: call need_to_revalidate (#8617)" This reverts commit 235c44a11f70d72168915af2d6504d0e63e880b3. Revert "TSHttpTxnCacheLookupStatusGet: handle cannot respond cases (#8545)" This reverts commit 080e236463490367ef94ce4e56fc95f287a917f5. --- src/traffic_server/InkAPI.cc | 15 +--- tests/Makefile.am | 1 - .../cache/cache-request-method.test.py | 30 +------ ...rint_cache_status_cache_post_disabled.gold | 7 -- ...print_cache_status_cache_post_enabled.gold | 10 --- tests/gold_tests/cache/plugins/Makefile.inc | 18 ---- .../cache/plugins/print_cache_status.cc | 89 ------------------- 7 files changed, 3 insertions(+), 167 deletions(-) delete mode 100644 tests/gold_tests/cache/gold/print_cache_status_cache_post_disabled.gold delete mode 100644 tests/gold_tests/cache/gold/print_cache_status_cache_post_enabled.gold delete mode 100644 tests/gold_tests/cache/plugins/Makefile.inc delete mode 100644 tests/gold_tests/cache/plugins/print_cache_status.cc diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc index f51c2a68d25..4db07a10c51 100644 --- a/src/traffic_server/InkAPI.cc +++ b/src/traffic_server/InkAPI.cc @@ -5429,20 +5429,7 @@ TSHttpTxnCacheLookupStatusGet(TSHttpTxn txnp, int *lookup_status) break; case HttpTransact::CACHE_LOOKUP_HIT_WARNING: case HttpTransact::CACHE_LOOKUP_HIT_FRESH: - if (HttpTransact::need_to_revalidate(&sm->t_state)) { - // Note that in this case the object was determined to be fresh but there - // was a problem with the cached object for this request per - // need_to_revalidate(). need_to_revalidate() checks for issues such as - // the need to authenticate with the origin or the incoming request - // method doesn't equal the method of the request for the cached - // response. Issues like this are more accurately described as a MISS - // rather than STALE because they are not due to freshness (timing) - // issues but rather to characteristics that make the cached object - // inappropriate as a response to this request. - *lookup_status = TS_CACHE_LOOKUP_MISS; - } else { - *lookup_status = TS_CACHE_LOOKUP_HIT_FRESH; - } + *lookup_status = TS_CACHE_LOOKUP_HIT_FRESH; break; case HttpTransact::CACHE_LOOKUP_SKIPPED: *lookup_status = TS_CACHE_LOOKUP_SKIPPED; diff --git a/tests/Makefile.am b/tests/Makefile.am index 81e7a1505e7..391078815e5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -31,7 +31,6 @@ AM_LDFLAGS += $(TS_PLUGIN_LD_FLAGS) AM_LDFLAGS += -rpath $(abs_builddir) include gold_tests/bigobj/Makefile.inc -include gold_tests/cache/plugins/Makefile.inc include gold_tests/continuations/plugins/Makefile.inc include gold_tests/chunked_encoding/Makefile.inc include gold_tests/pluginTest/tsapi/Makefile.inc diff --git a/tests/gold_tests/cache/cache-request-method.test.py b/tests/gold_tests/cache/cache-request-method.test.py index 2db75f82f6c..5715e812a14 100644 --- a/tests/gold_tests/cache/cache-request-method.test.py +++ b/tests/gold_tests/cache/cache-request-method.test.py @@ -17,8 +17,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import os - Test.Summary = ''' Verify correct caching behavior with respect to request method. ''' @@ -26,15 +24,11 @@ # Test 0: Verify correct POST response handling when caching POST responses is # disabled. ts = Test.MakeATSProcess("ts") -Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsBuildGoldTestsDir, - 'cache', 'plugins', '.libs', 'print_cache_status.so'), ts) -Test.Disk.File(os.path.join(ts.Variables.LOGDIR, 'print_cache_status.log'), - exists=True, content='gold/print_cache_status_cache_post_disabled.gold') replay_file = "replay/post_with_post_caching_disabled.replay.yaml" server = Test.MakeVerifierServerProcess("server0", replay_file) ts.Disk.records_config.update({ 'proxy.config.diags.debug.enabled': 1, - 'proxy.config.diags.debug.tags': 'http|cache|print_cache_status', + 'proxy.config.diags.debug.tags': 'http.*|cache.*', 'proxy.config.http.insert_age_in_response': 0, # Caching of POST responses is disabled by default. Verify default behavior @@ -49,26 +43,14 @@ tr.Processes.Default.StartBefore(ts) tr.AddVerifierClientProcess("client0", replay_file, http_ports=[ts.Variables.port]) -# Wait for log file to appear, then wait one extra second to make sure TS is done writing it. -test_run = Test.AddTestRun() -test_run.Processes.Default.Command = ( - os.path.join(Test.Variables.AtsTestToolsDir, 'condwait') + ' 60 1 -f ' + - os.path.join(ts.Variables.LOGDIR, 'print_cache_status.log') -) -test_run.Processes.Default.ReturnCode = 0 - # Test 1: Verify correct POST response handling when caching POST responses is # enabled. ts = Test.MakeATSProcess("ts-cache-post") -Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsBuildGoldTestsDir, - 'cache', 'plugins', '.libs', 'print_cache_status.so'), ts) -Test.Disk.File(os.path.join(ts.Variables.LOGDIR, 'print_cache_status.log'), - exists=True, content='gold/print_cache_status_cache_post_enabled.gold') replay_file = "replay/post_with_post_caching_enabled.replay.yaml" server = Test.MakeVerifierServerProcess("server1", replay_file) ts.Disk.records_config.update({ 'proxy.config.diags.debug.enabled': 1, - 'proxy.config.diags.debug.tags': 'http|cache|print_cache_status', + 'proxy.config.diags.debug.tags': 'http.*|cache.*', 'proxy.config.http.insert_age_in_response': 0, 'proxy.config.http.cache.post_method': 1, }) @@ -79,11 +61,3 @@ tr.Processes.Default.StartBefore(server) tr.Processes.Default.StartBefore(ts) tr.AddVerifierClientProcess("client1", replay_file, http_ports=[ts.Variables.port]) - -# Wait for log file to appear, then wait one extra second to make sure TS is done writing it. -test_run = Test.AddTestRun() -test_run.Processes.Default.Command = ( - os.path.join(Test.Variables.AtsTestToolsDir, 'condwait') + ' 60 1 -f ' + - os.path.join(ts.Variables.LOGDIR, 'print_cache_status.log') -) -test_run.Processes.Default.ReturnCode = 0 diff --git a/tests/gold_tests/cache/gold/print_cache_status_cache_post_disabled.gold b/tests/gold_tests/cache/gold/print_cache_status_cache_post_disabled.gold deleted file mode 100644 index 90e2e8d7d0f..00000000000 --- a/tests/gold_tests/cache/gold/print_cache_status_cache_post_disabled.gold +++ /dev/null @@ -1,7 +0,0 @@ -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_HIT_FRESH -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_HIT_FRESH -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_HIT_FRESH diff --git a/tests/gold_tests/cache/gold/print_cache_status_cache_post_enabled.gold b/tests/gold_tests/cache/gold/print_cache_status_cache_post_enabled.gold deleted file mode 100644 index f795b2951c6..00000000000 --- a/tests/gold_tests/cache/gold/print_cache_status_cache_post_enabled.gold +++ /dev/null @@ -1,10 +0,0 @@ -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_HIT_FRESH -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_HIT_FRESH -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_HIT_FRESH -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_MISS -`` Cache lookup status: TS_CACHE_LOOKUP_HIT_FRESH diff --git a/tests/gold_tests/cache/plugins/Makefile.inc b/tests/gold_tests/cache/plugins/Makefile.inc deleted file mode 100644 index 1b0b7baf920..00000000000 --- a/tests/gold_tests/cache/plugins/Makefile.inc +++ /dev/null @@ -1,18 +0,0 @@ -# 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. - -noinst_LTLIBRARIES += gold_tests/cache/plugins/print_cache_status.la -gold_tests_cache_plugins_print_cache_status_la_SOURCES = gold_tests/cache/plugins/print_cache_status.cc diff --git a/tests/gold_tests/cache/plugins/print_cache_status.cc b/tests/gold_tests/cache/plugins/print_cache_status.cc deleted file mode 100644 index 531b6aa4c6c..00000000000 --- a/tests/gold_tests/cache/plugins/print_cache_status.cc +++ /dev/null @@ -1,89 +0,0 @@ -/** - @file - @brief A plugin that prints the cache lookup status. - - @section license License - - 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. -*/ -#include // for debug -#include // for abort -#include // for PRId64 -#include -#include - -namespace -{ -constexpr char const *PLUGIN_NAME = "print_cache_status"; -static TSTextLogObject pluginlog; - -std::unordered_map lookup_status_to_string = { - {TS_CACHE_LOOKUP_MISS, "TS_CACHE_LOOKUP_MISS"}, - {TS_CACHE_LOOKUP_HIT_STALE, "TS_CACHE_LOOKUP_HIT_STALE"}, - {TS_CACHE_LOOKUP_HIT_FRESH, "TS_CACHE_LOOKUP_HIT_FRESH"}, - {TS_CACHE_LOOKUP_SKIPPED, "TS_CACHE_LOOKUP_SKIPPED"}, -}; - -int -global_handler(TSCont continuation, TSEvent event, void *data) -{ - TSHttpTxn txnp = static_cast(data); - - switch (event) { - case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: { - int obj_status = 0; - if (TS_ERROR == TSHttpTxnCacheLookupStatusGet(txnp, &obj_status)) { - TSError("[%s] TSHttpTxnCacheLookupStatusGet failed", PLUGIN_NAME); - } - TSTextLogObjectWrite(pluginlog, "Cache lookup status: %s", lookup_status_to_string[obj_status].data()); - } break; - - default: - TSError("[%s] Unexpected event: %d", PLUGIN_NAME, event); - return 0; - } - - TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); - - return 0; -} -} // anonymous namespace - -void -TSPluginInit(int argc, const char *argv[]) -{ - TSDebug(PLUGIN_NAME, "initializing plugin"); - - TSPluginRegistrationInfo info; - - info.plugin_name = PLUGIN_NAME; - info.vendor_name = "Apache"; - info.support_email = "bneradt@apache.org"; - - if (TSPluginRegister(&info) != TS_SUCCESS) { - TSError("[%s] Plugin registration failed.", PLUGIN_NAME); - } - TSAssert(TS_SUCCESS == TSTextLogObjectCreate(PLUGIN_NAME, TS_LOG_MODE_ADD_TIMESTAMP, &pluginlog)); - - TSCont contp = TSContCreate(global_handler, TSMutexCreate()); - if (contp == nullptr) { - TSError("[%s] could not create continuation.", PLUGIN_NAME); - std::abort(); - } else { - TSHttpHookAdd(TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, contp); - } -}