brbzull0 commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3956456921
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,44 @@ 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.
+ CHECK(rpc::test_remove_handler(_name));
+ }
+ }
Review Comment:
Leaving this one off.
As the comment notes, the destructor is already `noexcept(true)` --
implicitly,
because every member (`std::string`, `bool`) has a non-throwing destructor.
Adding
the keyword changes nothing observable: the terminate-on-throw behaviour is
identical either way.
The one thing it would buy is pinning the specification against a future
member
whose destructor can throw, which would otherwise flip the implicit spec
silently.
That is a real effect, but it does not seem worth it for a 30-line
test-local scope
guard holding a string and a bool.
Worth being precise about the risk it gestures at, since it is real but the
keyword
does not address it: `test_remove_handler` reaches
`Dispatcher::remove_handler`, which takes a `std::lock_guard<std::mutex>`,
and
`std::mutex::lock()` can throw `std::system_error`. So this destructor does
contain
a throwing call, and that path terminates -- with or without the annotation.
Avoiding
that would take a `try`/`catch(...)`, and in a test I would rather terminate
loudly
than swallow a failed cleanup, which is the thing this PR exists to make
visible.
Also noting for consistency: no destructor in the tree is currently marked
`noexcept`.
--
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]
