brbzull0 commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3956804363
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,45 @@ add_method_handler(const std::string &name, Func &&call)
{
return rpc::JsonRPCManager::instance().add_method_handler(name,
std::forward<Func>(call), nullptr, {});
}
+
+/// Registers a method handler and removes it when the scope ends.
+///
+/// Catch2 re-runs a TEST_CASE body once per leaf SECTION. Registering at the
top of the body and
+/// removing at the bottom only works while every assertion passes: REQUIRE is
fatal, so a failure
+/// inside a SECTION unwinds before the trailing removal and the handler
survives into the next
+/// SECTION's run, where re-registering it fails. One real failure then
reports as two, and the
+/// second points at a registration that was never the problem.
+class ScopedMethodHandler
+{
+public:
+ template <typename Func> ScopedMethodHandler(std::string name, Func &&call)
: _name{std::move(name)}
+ {
+ _registered = rpc::add_method_handler(_name, std::forward<Func>(call));
+ }
+
+ ~ScopedMethodHandler()
+ {
+ if (_registered) {
+ // CHECK rather than REQUIRE: this runs during stack unwinding when a
SECTION failed, and a
+ // fatal assertion there would abort instead of reporting.
+ INFO("handler: " << _name);
+ CHECK(rpc::test_remove_handler(_name));
Review Comment:
Done in 0c10ae00b, taking both halves of this.
`noexcept` added, and more to the point the body now upholds it -- a
`try`/`catch(...)`
around the removal, reporting via `std::cerr` since `CHECK` may be what
threw.
I had pushed back on this earlier on the grounds that terminating loudly
beats
swallowing a failed cleanup. That was wrong, for two reasons:
- The two paths are separate. A failed *removal* is a `CHECK` returning
false,
which is still reported; the `try`/`catch` only covers an *exception*,
which
today would end the entire test binary and lose all 14 test cases' results.
- The same file already solves this exact problem the same way --
`~ConfigRestorer()`
wraps its work in `try { ... } catch (...) {}` for precisely the reason
that a
scope guard's destructor runs when `REQUIRE` throws.
Concrete throw paths, for the record: `INFO` streams a `std::string` and
allocates,
and `test_remove_handler` reaches `Dispatcher::remove_handler`, which takes a
`std::lock_guard<std::mutex>`.
--
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]
