bryancall commented on code in PR #13733: URL: https://github.com/apache/trafficserver/pull/13733#discussion_r4363686297
########## tests/uranium_tests/pluginTest/crash_test/test_crash_test_throw.py: ########## @@ -0,0 +1,151 @@ +# 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 pathlib import Path +import re +import time + +from tools.uranium.services import ATS, ATSFactory, Curl, OriginServer, ServiceFactory + + +def configure_origin(services: ServiceFactory) -> OriginServer: + """Create the origin used to establish a healthy baseline request. + + :param services: Factory owning support services and their cleanup. + """ + + origin = services.origin("origin") + origin.add_response( + { + "headers": "GET / HTTP/1.1\r\nHost: example.com\r\n\r\n", + "body": "" + }, + { + "headers": "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: close\r\n\r\n", + "body": "Hello", + }, + ) + return origin + + +def configure_ats(ats_factory: ATSFactory, *, _origin: OriginServer) -> ATS: + """Load the plugin that throws out of a hook and enable the crash-log helper. + + The terminate handler aborts, so ATS is expected to exit on SIGABRT. + + :param _origin: Test-local origin configured by the test. + :param ats_factory: Factory for isolated Traffic Server instances. + """ + + ats = ats_factory.create("ts", return_code=-6, enable_cache=False) + ats.records.update( + { + "proxy.config.proxy_name": "test_proxy", + "proxy.config.url_remap.remap_required": 0, + "proxy.config.diags.debug.enabled": 1, + "proxy.config.diags.debug.tags": "crash_test", + "proxy.config.crash_log_helper": "traffic_crashlog", + }) + ats.copy_custom_plugin("{AtsBuildUraniumTestsDir}/pluginTest/crash_test/.libs/crash_test.so") + ats.plugin_config.add_line("crash_test.so") + ats.remap_config.add_line(f"map / http://127.0.0.1:{_origin.port}/";) + return ats + + +def verify_healthy_request(*, _ats: ATS, _curl: Curl) -> None: + """Prove ATS is serving traffic before triggering the fault. + + :param _ats: Test-local ats configured by the test. + :param _curl: Test-local curl configured by the test. + """ + + response = _curl.get( + _ats, + headers={"Host": "example.com"}, + options=f"--silent --output /dev/null --write-out '%{{http_code}}'", + ) + assert response.returncode == 0, response.output + assert response.stdout == "200" + + +def trigger_throw(*, _ats: ATS, _curl: Curl) -> None: + """Send the header that makes crash_test throw a std::runtime_error out of its hook. + + :param _ats: Test-local ats configured by the test. + :param _curl: Test-local curl configured by the test. + """ + + response = _curl.get( + _ats, + headers={ + "Host": "example.com", + "X-Crash-Test": "throw" + }, + options=f"--silent --output /dev/null", + ) + assert response.returncode in (52, 56), response.output + _ats.wait() + + +def wait_for_crash_log(*, _ats: ATS) -> Path: + """Wait until traffic_crashlog has finished writing its report. + + :param _ats: Test-local ats configured by the test. + """ + + deadline = time.monotonic() + 60 + while time.monotonic() < deadline: + matches = list(_ats.log_directory.glob("crash-*.log")) + if matches and "Other Non-Crashing Threads:" in matches[0].read_text(errors="replace"): + return matches[0] Review Comment: Fixed in f1c2215e13e6, which takes the newest crash log by mtime instead of `matches[0]`; 3c2e677acbe8 then made that a single `max()` pass. ########## src/traffic_cache_tool/CacheTool.cc: ########## @@ -1435,4 +1438,7 @@ main([[maybe_unused]] int argc, const char *argv[]) } return 0; +} catch (...) { + fprintf(stderr, "traffic_cache_tool: terminating on an unhandled %s\n", ts::describe_current_exception().c_str()); + return 1; } Review Comment: Leaving these as they are. The part worth sharing, building the message, is already one function, `ts::describe_current_exception()`. What is left in each handler is one line, and those lines differ on purpose: `traffic_logstats` writes to stdout and returns `EXIT_UNKNOWN` because Nagios-style wrappers read stdout, `traffic_layout` returns `EX_SOFTWARE`, `traffic_ctl` returns `CTRL_EX_ERROR`, and `traffic_top` restores the terminal first. A helper would take the program name, the stream and the exit code as arguments, so each call would be as long as the line it replaces. ########## src/tscore/unit_tests/test_Demangle.cc: ########## @@ -0,0 +1,80 @@ +/** @file + + Tests for the exception description helpers. + + @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 "tscore/Demangle.h" + +#include <catch2/catch_test_macros.hpp> + +#include <stdexcept> +#include <string> + +namespace +{ +struct NotAnException { +}; + +std::string +describe_thrown(auto const &thrown) +{ + try { + throw thrown; + } catch (...) { + return ts::describe_current_exception(); + } +} Review Comment: ATS builds as C++20: the top-level `CMakeLists.txt` sets `CMAKE_CXX_STANDARD` to 20 with `CMAKE_CXX_STANDARD_REQUIRED ON`, so an abbreviated function template is fine here. ########## src/tscore/unit_tests/test_Demangle.cc: ########## @@ -0,0 +1,80 @@ +/** @file + + Tests for the exception description helpers. + + @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 "tscore/Demangle.h" + +#include <catch2/catch_test_macros.hpp> + +#include <stdexcept> +#include <string> + +namespace +{ +struct NotAnException { +}; + +std::string +describe_thrown(auto const &thrown) +{ + try { + throw thrown; + } catch (...) { + return ts::describe_current_exception(); + } +} +} // namespace + +TEST_CASE("demangle", "[libts][Demangle]") +{ + CHECK(ts::demangle(typeid(int)) == "int"); + CHECK(ts::demangle(typeid(std::runtime_error)).find("runtime_error") != std::string::npos); + CHECK(ts::demangle(typeid(NotAnException)).find("NotAnException") != std::string::npos); +} + +TEST_CASE("describe_current_exception", "[libts][Demangle]") +{ + SECTION("a std::exception reports its type and message") + { + auto const text = describe_thrown(std::runtime_error{"boom"}); + + CHECK(text.find("runtime_error") != std::string::npos); + CHECK(text.ends_with(": boom")); Review Comment: Same as above: ATS requires C++20, so `std::string::ends_with` is available. ########## src/traffic_logcat/logcat.cc: ########## @@ -503,15 +506,17 @@ open_output_file(char *output_file_p) main -------------------------------------------------------------------------*/ +// At file scope so the exception handlers below can name these; a +// function-try-block handler is outside the function body's scope. +enum { + NO_ERROR = 0, + CMD_LINE_OPTION_ERROR = 1, + DATA_PROCESSING_ERROR = 2, +}; Review Comment: Moved the enum into the file's anonymous namespace in 344b2cb6a8e3. I kept it a plain enum rather than an `enum class`: its values are passed to `::exit()` and returned from `main()`, so a scoped enum would need a cast at each of those. ########## tests/uranium_tests/pluginTest/crash_test/test_crash_test_throw.py: ########## @@ -0,0 +1,151 @@ +# 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 pathlib import Path +import re +import time + +from tools.uranium.services import ATS, ATSFactory, Curl, OriginServer, ServiceFactory + + +def configure_origin(services: ServiceFactory) -> OriginServer: + """Create the origin used to establish a healthy baseline request. + + :param services: Factory owning support services and their cleanup. + """ + + origin = services.origin("origin") + origin.add_response( + { + "headers": "GET / HTTP/1.1\r\nHost: example.com\r\n\r\n", + "body": "" + }, + { + "headers": "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: close\r\n\r\n", + "body": "Hello", + }, + ) + return origin + + +def configure_ats(ats_factory: ATSFactory, *, _origin: OriginServer) -> ATS: + """Load the plugin that throws out of a hook and enable the crash-log helper. + + The terminate handler aborts, so ATS is expected to exit on SIGABRT. + + :param _origin: Test-local origin configured by the test. + :param ats_factory: Factory for isolated Traffic Server instances. + """ + + ats = ats_factory.create("ts", return_code=-6, enable_cache=False) Review Comment: Changed in 344b2cb6a8e3 to `return_code=-signal.SIGABRT`. -- 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]
