Copilot commented on code in PR #13733:
URL: https://github.com/apache/trafficserver/pull/13733#discussion_r4355621387
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1971,6 +1973,24 @@ configure_io_uring()
}
#endif
+// Runs when an exception escapes on any thread. The terminate handler runs
+// before the stack unwinds, so aborting from here keeps the throwing frame in
+// the core. The one exception is a throw that leaves a noexcept function: the
+// runtime unwinds to that function's frame before it calls terminate, so the
+// core loses the frames between the throw and it. Catching in main() instead
+// would unwind those frames first and would miss other threads entirely.
main()
+// installs this before init_system() registers the crash handler, and before
+// crash_logger_init() starts the crash logger. A throw in that window still
+// aborts and dumps a core, but writes no crash log.
+[[noreturn]] void
+abort_on_uncaught_exception()
+{
+ if (std::current_exception()) {
+ ink_abort("traffic_server: terminating on an unhandled %s",
ts::describe_current_exception().c_str());
+ }
+ ink_abort("traffic_server: terminate called without an active exception");
+}
Review Comment:
`abort_on_uncaught_exception()` calls `ts::describe_current_exception()`,
which (via demangling and `std::string` composition) can allocate memory. In
terminate paths triggered by OOM or heap corruption, additional allocations can
fail, recurse, or deadlock, making crash reporting less reliable. Consider
offering a no-allocation reporting path for terminate handlers (e.g., avoid
demangling and avoid `std::string` concatenation; print `type.name()`/`what()`
directly via a minimal writer), keeping the current rich formatting for normal
catch blocks.
##########
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:
The PR adds nearly identical catch-and-report blocks across many `main()`
functions. This duplication increases the chance of formatting/behavior drift
over time (e.g., stdout vs stderr, exit code mapping, message wording).
Consider factoring the shared reporting into a small helper in `tscore` (e.g.,
`ts::print_unhandled_exception(program_name, FILE*)`) so each tool only selects
the exit code and output stream.
##########
src/traffic_top/traffic_top.cc:
##########
@@ -412,6 +432,7 @@ main([[maybe_unused]] int argc, const char **argv)
const string &host = stats.getHost();
initscr();
Review Comment:
`initscr()` returns a `WINDOW*` and can fail. The code sets `curses_started
= true` unconditionally, which can later make `restore_terminal()` call
`endwin()` even though curses was never successfully initialized (undefined
behavior per the comment above). Capture the return value of `initscr()` and
only set `curses_started = true` when it succeeds; if it fails, report/return
an error without calling curses teardown.
##########
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 = sorted(_ats.log_directory.glob("crash-*.log"), key=lambda p:
p.stat().st_mtime)
+ if matches and "Other Non-Crashing Threads:" in
matches[-1].read_text(errors="replace"):
+ return matches[-1]
Review Comment:
This loop re-stats and fully sorts all `crash-*.log` files every 100ms. In
directories with many crash logs (e.g., after repeated runs), this can become
unnecessarily expensive and slow/flaky. A lower-overhead approach is to scan
once for the newest file (e.g., `max(..., key=mtime)`), or track the
newest-seen mtime/path between iterations to avoid repeated sorting.
--
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]
