cmcfarlen commented on code in PR #13658:
URL: https://github.com/apache/trafficserver/pull/13658#discussion_r3973708836
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -185,12 +185,15 @@ struct RPCServerTestListener : Catch::EventListenerBase {
R"(", "backlog": 5,"max_retry_on_transient_errors": 64,
"incoming_request_max_size": 32000 }}})"};
YAML::Node configNode = YAML::Load(confStr);
serverConfig.load(configNode["rpc"]);
+ // Report this loudly: otherwise every socket test just fails later with a
+ // confusing "no such file" on the socket path, which is especially noisy
+ // now that each test case runs as its own process.
try {
jsonrpcServer = new rpc::RPCServer(serverConfig);
jsonrpcServer->start_thread();
} catch (std::exception const &ex) {
- Dbg(dbg_ctl, "Oops: %s", ex.what());
+ std::fprintf(stderr, "Failed to start the JSONRPC test server on %s:
%s\n", sockPath.c_str(), ex.what());
}
Review Comment:
Covered in the earlier thread on this same code, but two specifics on this
version of the comment:
**Fail-fast:** I tried it and backed it out. A failing `REQUIRE` inside a
Catch2 listener callback **segfaults** — minimal repro against the vendored
v3.9.1:
```cpp
struct Listener : Catch::EventListenerBase {
using EventListenerBase::EventListenerBase;
void testRunStarting(Catch::TestRunInfo const &) override {
REQUIRE(false); }
};
CATCH_REGISTER_LISTENER(Listener)
TEST_CASE("some case") { CHECK(1 == 1); }
```
→ `exit 139`, no output. Rethrowing does work, but it fails all 14 cases
including the 6 that never touch the server (`Layout::create()`,
`RecProcessInit()` and `eventProcessor.start()` all succeeded), so it converts
valid passes into failures. The printed message already lands as the first line
of the failing test's captured output, which is what the previously-swallowed
`Dbg` did not do.
**The leak:** I don't think there is one. If the constructor throws, the
assignment never happens and `jsonrpcServer` stays null. If `start_thread()`
throws, `jsonrpcServer` is non-null and `testRunEnded()` does `if
(jsonrpcServer) { delete jsonrpcServer; }`. Either path is covered — unless
you're seeing something I've missed.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -185,12 +185,15 @@ struct RPCServerTestListener : Catch::EventListenerBase {
R"(", "backlog": 5,"max_retry_on_transient_errors": 64,
"incoming_request_max_size": 32000 }}})"};
YAML::Node configNode = YAML::Load(confStr);
serverConfig.load(configNode["rpc"]);
+ // Report this loudly: otherwise every socket test just fails later with a
+ // confusing "no such file" on the socket path, which is especially noisy
+ // now that each test case runs as its own process.
try {
jsonrpcServer = new rpc::RPCServer(serverConfig);
jsonrpcServer->start_thread();
} catch (std::exception const &ex) {
- Dbg(dbg_ctl, "Oops: %s", ex.what());
+ std::fprintf(stderr, "Failed to start the JSONRPC test server on %s:
%s\n", sockPath.c_str(), ex.what());
Review Comment:
Third instance of this one — see the threads above. Short version: `REQUIRE`
in a Catch2 listener segfaults (verified with a minimal repro), and rethrowing
would fail the 6 cases that never touch the server. The printed message already
surfaces the root cause as the first line of the failing test output.
--
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]
