diff --git a/include/proxy/http/HttpTransact.h b/include/proxy/http/HttpTransact.h index e673005a8c2..7ca984f00ba 100644 --- a/include/proxy/http/HttpTransact.h +++ b/include/proxy/http/HttpTransact.h @@ -1025,7 +1025,6 @@ class HttpTransact 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 dbdc1456c0b..225296bf485 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 c6fb868cb9c..8c1fb59570f 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 45ec9e03c76..00000000000 --- 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 4ca60e1926d..00000000000 --- 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. - - -@TestRedirectToSameOriginOnCache.runner("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.")