This is an automated email from the ASF dual-hosted git repository.
jvanderzee pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/10.0.x by this push:
new 78b930e7fe Revert "Reuse prepared cache write on redirected request
(#11542)" (#11706)
78b930e7fe is described below
commit 78b930e7fe40b269d9c059c9c9e8a9c567dd89d0
Author: JosiahWI <[email protected]>
AuthorDate: Fri Aug 16 12:33:42 2024 -0500
Revert "Reuse prepared cache write on redirected request (#11542)" (#11706)
This reverts commit 6d55a20149dcfa9b2cf0803cda29bdab3ebf0639.
---
include/proxy/http/HttpTransact.h | 1 -
src/proxy/http/HttpSM.cc | 3 +-
src/proxy/http/HttpTransact.cc | 31 ++---
.../redirect_to_same_origin_on_cache.replay.yaml | 65 ----------
.../redirect_to_same_origin_on_cache.test.py | 140 ---------------------
5 files changed, 9 insertions(+), 231 deletions(-)
diff --git a/include/proxy/http/HttpTransact.h
b/include/proxy/http/HttpTransact.h
index e673005a8c..7ca984f00b 100644
--- a/include/proxy/http/HttpTransact.h
+++ b/include/proxy/http/HttpTransact.h
@@ -1025,7 +1025,6 @@ public:
static void HandleCacheOpenReadHitFreshness(State *s);
static void HandleCacheOpenReadHit(State *s);
static void HandleCacheOpenReadMiss(State *s);
- static void set_cache_prepare_write_action_for_new_request(State *s);
static void build_response_from_cache(State *s, HTTPWarningCode
warning_code);
static void handle_cache_write_lock(State *s);
static void HandleResponse(State *s);
diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc
index dbdc1456c0..225296bf48 100644
--- a/src/proxy/http/HttpSM.cc
+++ b/src/proxy/http/HttpSM.cc
@@ -8057,8 +8057,9 @@ HttpSM::set_next_state()
}
case HttpTransact::SM_ACTION_CACHE_ISSUE_WRITE: {
- ink_assert(cache_sm.cache_write_vc == nullptr);
+ ink_assert((cache_sm.cache_write_vc == nullptr) ||
t_state.redirect_info.redirect_in_process);
HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_cache_open_write);
+
do_cache_prepare_write();
break;
}
diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc
index c6fb868cb9..8c1fb59570 100644
--- a/src/proxy/http/HttpTransact.cc
+++ b/src/proxy/http/HttpTransact.cc
@@ -2171,7 +2171,12 @@ HttpTransact::DecideCacheLookup(State *s)
if (s->redirect_info.redirect_in_process) {
// without calling out the CACHE_LOOKUP_COMPLETE_HOOK
if (s->txn_conf->cache_http) {
- HttpTransact::set_cache_prepare_write_action_for_new_request(s);
+ if (s->cache_info.write_lock_state == CACHE_WL_FAIL) {
+ s->cache_info.action = CACHE_PREPARE_TO_WRITE;
+ s->cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT;
+ } else if (s->cache_info.write_lock_state == CACHE_WL_SUCCESS) {
+ s->cache_info.action = CACHE_DO_WRITE;
+ }
}
LookupSkipOpenServer(s);
} else {
@@ -3286,7 +3291,7 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
} else if (s->api_server_response_no_store) { // plugin may have decided not
to cache the response
s->cache_info.action = CACHE_DO_NO_ACTION;
} else {
- HttpTransact::set_cache_prepare_write_action_for_new_request(s);
+ s->cache_info.action = CACHE_PREPARE_TO_WRITE;
}
///////////////////////////////////////////////////////////////
@@ -3341,28 +3346,6 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
return;
}
-void
-HttpTransact::set_cache_prepare_write_action_for_new_request(State *s)
-{
- // This method must be called no more than one time per request. It should
- // not be called for non-cacheable requests.
- if (s->cache_info.write_lock_state == CACHE_WL_SUCCESS) {
- // If and only if this is a redirected request, we may have already
- // prepared a cache write (during the handling of the previous request
- // which got the 3xx response) and can safely re-use it. Otherwise, we
- // risk storing the response under the wrong cache key. This is a release
- // assert because the correct behavior would be to prepare a new write,
- // but we can't do that because we failed to release the lock. To recover
- // we would have to tell the state machine to abort its write, and we
- // don't have a state for that.
- ink_release_assert(s->redirect_info.redirect_in_process);
- s->cache_info.action = CACHE_DO_WRITE;
- } else {
- s->cache_info.action = CACHE_PREPARE_TO_WRITE;
- s->cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT;
- }
-}
-
///////////////////////////////////////////////////////////////////////////////
// Name : OriginServerRawOpen
// Description: called for ssl tunneling
diff --git
a/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml
b/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml
deleted file mode 100644
index 45ec9e03c7..0000000000
--- a/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml
+++ /dev/null
@@ -1,65 +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.
-
-meta:
- version: "1.0"
-
-sessions:
-- protocol:
- - name: http
- version: 1
- - name: tcp
- - name: ip
-
- transactions:
- - client-request:
- method: "GET"
- version: "1.1"
- url: /a/path/resource
- headers:
- fields:
- - [ Host, oof.com ]
- - [ Connection, keep-alive ]
- - [ Content-Length, 0 ]
-
- server-response:
- status: 307
- headers:
- fields:
- - [ location, /a/new/path/resource ]
- - [ content-length, 0 ]
-
- proxy-response:
- status: 200
- - client-request:
- method: "GET"
- version: "1.1"
- url: /a/new/path/resource
- headers:
- fields:
- - [ Host, oof.com ]
- - [ Connection, keep-alive ]
- - [ Content-Length, 0 ]
-
- server-response:
- status: 200
- reason: OK
- headers:
- fields:
- - [ content-length, 16 ]
-
- proxy-response:
- status: 200
diff --git a/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py
b/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py
deleted file mode 100644
index 4ca60e1926..0000000000
--- a/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py
+++ /dev/null
@@ -1,140 +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.
-
-import functools
-from typing import Any, Callable, Dict, Optional
-
-from ports import get_port
-
-Test.Summary = 'Tests SNI port-based routing'
-
-TestParams = Dict[str, Any]
-
-
-class TestRedirectToSameOriginOnCache:
- """Configure a test for reproducing #9275."""
-
- replay_filepath_one: str = "redirect_to_same_origin_on_cache.replay.yaml"
- client_counter: int = 0
- server_counter: int = 0
- ts_counter: int = 0
-
- def __init__(self, name: str, /, autorun) -> None:
- """Initialize the test.
-
- :param name: The name of the test.
- """
- self.name = name
- self.autorun = autorun
-
- def _init_run(self) -> "TestRun":
- """Initialize processes for the test run."""
-
- tr = Test.AddTestRun(self.name)
- server_one = TestRedirectToSameOriginOnCache.configure_server(tr,
"oof.com")
- self._configure_traffic_server(tr, server_one)
- dns = TestRedirectToSameOriginOnCache.configure_dns(tr, server_one,
self._dns_port)
-
- tr.Processes.Default.StartBefore(server_one)
- tr.Processes.Default.StartBefore(dns)
- tr.Processes.Default.StartBefore(self._ts)
-
- return {
- "tr": tr,
- "ts": self._ts,
- "server_one": server_one,
- "port_one": self._port_one,
- }
-
- @classmethod
- def runner(cls, name: str, autorun: bool = True) -> Optional[Callable]:
- """Create a runner for a test case.
-
- :param autorun: Run the test case once it's set up. Default is True.
- """
- test = cls(name, autorun=autorun)._prepare_test_case
- return test
-
- def _prepare_test_case(self, func: Callable) -> Callable:
- """Set up a test case and possibly run it.
-
- :param func: The test case to set up.
- """
- functools.wraps(func)
- test_params = self._init_run()
-
- def wrapper(*args, **kwargs) -> Any:
- return func(test_params, *args, **kwargs)
-
- if self.autorun:
- wrapper()
- return wrapper
-
- @staticmethod
- def configure_server(tr: "TestRun", domain: str):
- server = tr.AddVerifierServerProcess(
- f"server{TestRedirectToSameOriginOnCache.server_counter}.{domain}",
- TestRedirectToSameOriginOnCache.replay_filepath_one,
- other_args="--format \"{url}\"")
- TestRedirectToSameOriginOnCache.server_counter += 1
-
- return server
-
- @staticmethod
- def configure_dns(tr: "TestRun", server_one: "Process", dns_port: int):
- dns = tr.MakeDNServer("dns", port=dns_port, default="127.0.0.1")
- return dns
-
- def _configure_traffic_server(self, tr: "TestRun", server_one: "Process"):
- """Configure Traffic Server.
-
- :param tr: The TestRun object to associate the ts process with.
- """
- ts = tr.MakeATSProcess(
- f"ts-{TestRedirectToSameOriginOnCache.ts_counter}",
select_ports=False, enable_tls=True, enable_cache=True)
- TestRedirectToSameOriginOnCache.ts_counter += 1
-
- self._port_one = get_port(ts, "PortOne")
- self._dns_port = get_port(ts, "DNSPort")
- ts.Disk.records_config.update(
- {
- 'proxy.config.http.server_ports': f"{self._port_one}",
- 'proxy.config.diags.debug.enabled': 1,
- 'proxy.config.diags.debug.tags':
"cache|dns|http|redirect|remap",
- 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns_port}",
- 'proxy.config.dns.resolv_conf': 'NULL',
- 'proxy.config.http.redirect.actions': "self:follow",
- 'proxy.config.http.number_of_redirections': 1,
- })
-
- ts.Disk.remap_config.AddLine(f"map oof.com
http://oof.backend.com:{server_one.Variables.http_port}";)
-
- self._ts = ts
-
-
-# Tests start.
-
-
[email protected]("Redirect to same origin with cache")
-def test1(params: TestParams) -> None:
- client = params["tr"].AddVerifierClientProcess(
- f"client0",
- TestRedirectToSameOriginOnCache.replay_filepath_one,
- http_ports=[params["port_one"]],
- other_args="--format \"{url}\" --keys \"/a/path/resource\"")
-
- params["tr"].Processes.Default.ReturnCode = 0
- params["tr"].Processes.Default.Streams.stdout.Content +=
Testers.IncludesExpression("200 OK", "We should get the resource.")
