bryancall commented on code in PR #13733:
URL: https://github.com/apache/trafficserver/pull/13733#discussion_r4363214666
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1971,6 +1973,22 @@ configure_io_uring()
}
#endif
+// Runs when an exception escapes on any thread. The terminate handler runs
Review Comment:
Added in f1c2215e13e6: the comment now says that a throw leaving a
`noexcept` function is unwound to that function's frame before `std::terminate`
runs, so the core loses the frames between the throw and it.
##########
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:
Both `ts::demangle()` and `ts::describe_current_exception()` are `noexcept`
and catch every failure inside them. If demangling or building the message
throws `std::bad_alloc`, they fall back to `"<unknown>"` or `"exception"`, and
both of those fit in the small-string buffer, so the fallback itself allocates
nothing. Under memory pressure the handler still aborts, just with a less
specific message. Heap corruption can stall any path here, including the
logging inside `ink_abort()`, and a plain writer would not fix that, so I am
keeping the richer message.
##########
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:
ncurses' `initscr()` does not return on failure: it writes an error to
stderr and exits (see `initscr(3X)`). So when the call returns, curses is
initialized, and `curses_started` is never set after a failed initialization.
##########
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:
Changed in 3c2e677acbe8: each poll now takes the newest file with a single
`max(..., key=mtime, default=None)` instead of sorting the whole glob.
--
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]
