JakeChampion commented on code in PR #13971:
URL: https://github.com/apache/trafficserver/pull/13971#discussion_r4457807744
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2613,6 +2613,17 @@ HttpSM::state_cache_open_write(int event, void *data)
{
STATE_ENTER(state_cache_open_write, event);
+ // The tunnel that feeds the response transform can complete while the cache
+ // write for the transformed copy is still open. This event does not come
from
+ // the cache, so the open write is still pending. Do not touch pending_action
+ // here: an assignment cancels the pending open write, its callback is then
+ // dropped, and the transaction stalls until the client times out.
+ if (event == HTTP_TUNNEL_EVENT_DONE) {
+ ink_release_assert(t_state.next_action ==
HttpTransact::StateMachineAction_t::CACHE_ISSUE_WRITE_TRANSFORM);
+ state_common_wait_for_transform_read(&transform_info,
&HttpSM::tunnel_handler, event, data);
Review Comment:
Agreed. Before this change the stray assignment cancelled the write, so the
error path did not see it. Fixed in 070c3ee41: the error branch of
`state_common_wait_for_transform_read` now cancels `pending_action` before it
cleans up and builds the error response. The truncated-but-forwardable case
still returns early and keeps the write pending.
##########
tests/uranium_tests/pluginTest/compress/test_compress_transform_cache_write.py:
##########
@@ -0,0 +1,94 @@
+# 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.
+
+from concurrent.futures import ThreadPoolExecutor
+from pathlib import Path
+import sys
+
+import pytest
+
+from tools.uranium.services import ATSFactory, Curl, ServiceFactory
+
+PATHS = 10
+CONCURRENCY = 6
+CLIENT_MAX_TIME = 10
+
+
+def test_compress_transform_cache_write(ats_factory: ATSFactory, services:
ServiceFactory, curl: Curl) -> None:
+ """Concurrent transforms of a cached identity object must all complete.
+
+ Each request that accepts br hits the stored uncompressed object, runs it
+ through compress.so, and opens a cache write for the compressed copy. With
+ several of these requests at the same time the open write retries on the
+ write lock. If the cache-to-transform tunnel completes during the retry,
+ HTTP_TUNNEL_EVENT_DONE reaches HttpSM::state_cache_open_write. That
+ handler must not cancel the pending open write, or the transaction never
+ sends a response.
+
+ :param ats_factory: Factory for isolated proxy instances.
+ :param services: Factory owning the origin.
+ :param curl: Transport-aware client.
+ """
+ directory = Path(__file__).parent
+ port = services.allocate_port()
+ origin = services.process(
+ "origin",
+ (sys.executable, directory / "compress_large_origin.py", "--port",
str(port)),
+ ready_port=port,
+ )
+ ats = ats_factory.create("ts", enable_cache=True)
+ if not ats.plugin_exists("compress.so"):
+ pytest.skip("compress.so is required")
Review Comment:
Fixed in 070c3ee41. The test now skips on `ats.has_feature("TS_HAS_BROTLI")`
before ATS starts, and the `br` probe is an assertion.
##########
tests/uranium_tests/pluginTest/compress/test_compress_transform_cache_write.py:
##########
@@ -0,0 +1,94 @@
+# 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.
+
+from concurrent.futures import ThreadPoolExecutor
+from pathlib import Path
+import sys
+
+import pytest
+
+from tools.uranium.services import ATSFactory, Curl, ServiceFactory
+
+PATHS = 10
+CONCURRENCY = 6
+CLIENT_MAX_TIME = 10
+
+
+def test_compress_transform_cache_write(ats_factory: ATSFactory, services:
ServiceFactory, curl: Curl) -> None:
+ """Concurrent transforms of a cached identity object must all complete.
+
+ Each request that accepts br hits the stored uncompressed object, runs it
+ through compress.so, and opens a cache write for the compressed copy. With
+ several of these requests at the same time the open write retries on the
+ write lock. If the cache-to-transform tunnel completes during the retry,
+ HTTP_TUNNEL_EVENT_DONE reaches HttpSM::state_cache_open_write. That
+ handler must not cancel the pending open write, or the transaction never
+ sends a response.
+
+ :param ats_factory: Factory for isolated proxy instances.
+ :param services: Factory owning the origin.
+ :param curl: Transport-aware client.
+ """
+ directory = Path(__file__).parent
+ port = services.allocate_port()
+ origin = services.process(
+ "origin",
+ (sys.executable, directory / "compress_large_origin.py", "--port",
str(port)),
+ ready_port=port,
+ )
+ ats = ats_factory.create("ts", enable_cache=True)
+ if not ats.plugin_exists("compress.so"):
+ pytest.skip("compress.so is required")
+ ats.records.update(
+ {
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "compress",
+ # Make the open write for the transformed copy wait long enough
+ # for the transform tunnel to complete first.
+ "proxy.config.http.cache.max_open_read_retries": 5,
Review Comment:
Fixed in 070c3ee41: the test sets
`proxy.config.http.cache.ignore_accept_encoding_mismatch: 1`. The stall also
occurs on a miss, because the origin-to-transform tunnel can complete during
the write retry too. But this makes the test use the cache-to-transform path
that it describes.
##########
tests/uranium_tests/pluginTest/compress/test_compress_transform_cache_write.py:
##########
@@ -0,0 +1,94 @@
+# 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.
+
+from concurrent.futures import ThreadPoolExecutor
+from pathlib import Path
+import sys
+
+import pytest
+
+from tools.uranium.services import ATSFactory, Curl, ServiceFactory
+
+PATHS = 10
+CONCURRENCY = 6
+CLIENT_MAX_TIME = 10
+
+
+def test_compress_transform_cache_write(ats_factory: ATSFactory, services:
ServiceFactory, curl: Curl) -> None:
+ """Concurrent transforms of a cached identity object must all complete.
+
+ Each request that accepts br hits the stored uncompressed object, runs it
+ through compress.so, and opens a cache write for the compressed copy. With
+ several of these requests at the same time the open write retries on the
+ write lock. If the cache-to-transform tunnel completes during the retry,
+ HTTP_TUNNEL_EVENT_DONE reaches HttpSM::state_cache_open_write. That
+ handler must not cancel the pending open write, or the transaction never
+ sends a response.
+
+ :param ats_factory: Factory for isolated proxy instances.
+ :param services: Factory owning the origin.
+ :param curl: Transport-aware client.
+ """
+ directory = Path(__file__).parent
+ port = services.allocate_port()
+ origin = services.process(
+ "origin",
+ (sys.executable, directory / "compress_large_origin.py", "--port",
str(port)),
+ ready_port=port,
+ )
+ ats = ats_factory.create("ts", enable_cache=True)
+ if not ats.plugin_exists("compress.so"):
+ pytest.skip("compress.so is required")
+ ats.records.update(
+ {
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "compress",
+ # Make the open write for the transformed copy wait long enough
+ # for the transform tunnel to complete first.
+ "proxy.config.http.cache.max_open_read_retries": 5,
+ "proxy.config.http.cache.open_read_retry_time": 100,
+ })
+ config = directory / "etc" / "cache-true-transform-write.config"
+ ats.copy_to_config(config)
+ ats.remap_config.add_line(f"map / http://127.0.0.1:{port}/
@plugin=compress.so @pparam={ats.config_directory / config.name}")
+ origin.start()
+ ats.start()
+
+ def fetch(url: str, accept_encoding: str | None) -> tuple[str, str]:
+ """Return the status code and Content-Encoding curl saw, or 000."""
+ header = f"--header 'Accept-Encoding: {accept_encoding}' " if
accept_encoding else ""
+ result = curl.run_for(
+ ats, f"--http1.1 --silent --output /dev/null --max-time
{CLIENT_MAX_TIME} {header}"
+ f"--write-out '%{{http_code}} %header{{content-encoding}}' {url}",
+ timeout=CLIENT_MAX_TIME + 5)
+ status, _, encoding = result.stdout.strip().partition(" ")
Review Comment:
Fixed in 070c3ee41. `fetch()` now reports a non-zero curl exit code as a
failure. With this, the test fails on unpatched master (25 of 60 requests ended
with curl exit 28) and passes with the fix, both in the Fedora 44 test
container.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
