diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index d514fcc..cac5719 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -107,7 +107,12 @@ jobs: run: | ctest --output-on-failure --verbose env: - ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + # exitcode=86 distinguishes a leak from a failing assertion: LSan's default + # is 1, the same code a test returns when it fails, so the two are otherwise + # indistinguishable in the log. halt_on_error=1 matters just as much as + # detect_leaks=1 -- with detect_leaks alone the leak is reported but the + # process still exits 0, which CI reads as a pass. + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1:exitcode=86 LSAN_OPTIONS: suppressions=../lsan.supp # Undefined Behavior Sanitizer @@ -359,7 +364,7 @@ jobs: run: | ctest -L fuzz --output-on-failure -E FuzzSmoke env: - ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1:exitcode=86 LSAN_OPTIONS: suppressions=../lsan.supp - name: Fuzz smoke test @@ -367,7 +372,7 @@ jobs: run: | ctest -L fuzz --output-on-failure -R FuzzSmoke env: - ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1:exitcode=86 LSAN_OPTIONS: suppressions=../lsan.supp # A crash leaves a reproducer behind; upload it so the failure is actionable diff --git a/CHANGELOG.md b/CHANGELOG.md index 90db617..a40e0d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- `tests/test_web_layer.cpp` covers the Drogon adapter and the embedded pages end to + end: it starts the real server, drives it over raw-socket HTTP and shuts it down. + Until now every test target was core-only, so `drogon_adapter.cpp` and + `web_resources.cpp` were compiled under ASan but executed by nothing that ctest + ran -- the sanitizer build only looked like it covered the web layer. The server is + started once per suite because `drogon::app()` is a bind-once singleton: a + per-test SetUp/TearDown segfaults in `ListenerManager::addListener` on the second + test. Port 0 plus `getListeners()` keeps parallel runs from colliding. +- The sanitizer steps set `ASAN_OPTIONS=...:exitcode=86`, so a leak is distinguishable + from a failed assertion in the log. LSan exits 1 by default, which is the same code + a test returns when an assertion fails. - libFuzzer targets for the parsers, behind `-DREMOTE_PROFILER_BUILD_FUZZERS=ON` (clang only, off by default): `fuzz_renderer_output`, `fuzz_result_parsing`, `fuzz_http_params`, with a committed seed corpus each. `ctest -L fuzz` replays the diff --git a/CMakeLists.txt b/CMakeLists.txt index 23dfa6a..62a6956 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -308,6 +308,21 @@ if(REMOTE_PROFILER_BUILD_TESTS) ) add_test(NAME ThreadStacksTest COMMAND test_thread_stacks) + # End-to-end web layer (Drogon adapter + embedded resources). Only built when + # the web layer is enabled, since it is the only test that links it. + if(REMOTE_PROFILER_ENABLE_WEB) + add_executable(test_web_layer tests/test_web_layer.cpp) + target_link_libraries(test_web_layer + PRIVATE + profiler_web + Drogon::Drogon + GTest::gtest + GTest::gtest_main + pthread + ) + add_test(NAME WebLayerTest COMMAND test_web_layer) + endif() + message(STATUS "Tests will be built") else() message(STATUS "Tests disabled") diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 20e19f7..e71604c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -314,6 +314,17 @@ build/fuzz/fuzz_renderer_output -max_total_time=300 tests/fuzz/corpus/fuzz_rende - **Sanitizer**：`ctest` 默认构建不会报内存泄漏与未定义行为。曾经有一个 `stopHeapProfiler()` 的 LeakSanitizer 报告只被 CI 发现，因为本地从没跑过 `-fsanitize=address`。 + + ⚠️ 想靠**退出码**判断有没有泄漏，两个参数都要给，只给第一个不够： + + | 设置 | 泄漏报告 | 退出码 | + |------|----------|--------| + | `detect_leaks=1`（`halt_on_error` 默认 0） | ✅ 打印 | **0** ← 泄漏了但 CI 视为通过 | + | `detect_leaks=1:halt_on_error=1` | ✅ 打印 | 非零 ✅ | + | 再加 `exitcode=86` | ✅ 打印 | **86** ← 与断言失败（1）可区分 | + + 另外两条限制：进程被 `_exit()`/信号杀死时 LSan 没机会运行；被抑制规则覆盖的泄漏 + （如 `lsan.supp` 里的 tcmalloc 初始化）不会让退出码非零。 - **Web UI**：`curl` **无法执行页面 JavaScript**，因此看不到"按钮点击报 ReferenceError""按钮卡在 disabled""下载其实没开始"这类问题。前端改动必须用 真实浏览器验证后才算完成。 diff --git a/scripts/check-sanitizers.sh b/scripts/check-sanitizers.sh index e5f7215..253fe9f 100755 --- a/scripts/check-sanitizers.sh +++ b/scripts/check-sanitizers.sh @@ -58,8 +58,11 @@ run_asan() { -DCMAKE_MODULE_LINKER_FLAGS="-fsanitize=address" \ "${extra_configure_args[@]}" cmake --build "$dir" -j"$JOBS" - # detect_leaks + halt_on_error: a leak must fail the run, as it does in CI. - ( cd "$dir" && ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 \ + # detect_leaks reports a leak; halt_on_error is what makes the process fail + # because of it (with detect_leaks alone the report is printed and the exit code + # stays 0). exitcode=86 then separates a leak from a failing assertion, since + # LSan's default 1 is the same code a test uses when an assertion fails. + ( cd "$dir" && ASAN_OPTIONS=detect_leaks=1:halt_on_error=1:exitcode=86 \ LSAN_OPTIONS="suppressions=$PROJECT_ROOT/lsan.supp" \ ctest --output-on-failure "${ctest_parallel[@]}" ) } diff --git a/tests/test_full_flow.cpp b/tests/test_full_flow.cpp index a26452a..f57dce2 100644 --- a/tests/test_full_flow.cpp +++ b/tests/test_full_flow.cpp @@ -16,14 +16,20 @@ namespace { /// profile must therefore generate real CPU work for the whole window instead of /// sleeping through it -- otherwise they fail whenever the machine happens to be /// idle, which is exactly how this test failed under parallel ctest. -void burnCPUFor(int ms) { +uint64_t burnCPUFor(int ms) { const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(ms); - volatile uint64_t sink = 0; + uint64_t sink = 0; while (std::chrono::steady_clock::now() < deadline) { for (int i = 0; i < 20000; ++i) { sink += static_cast(i) * 2654435761u; } } + // Returning it rather than marking it unused: callers assert it is non-zero, so + // the accumulation is observable and the compiler cannot elide it. A `volatile` + // local would silence the optimizer but still trip -Wunused-but-set-variable + // (GCC sees through volatile for that warning, and the warnings-check job builds + // with -Wall -Wextra -Werror). + return sink; } } // namespace @@ -38,7 +44,7 @@ TEST(FullFlowTest, GperftoolsGeneratesValidProfile) { // 运行足够的工作负载 std::cout << "Running workload...\n"; - volatile long long result = 0; + long long result = 0; for (int i = 0; i < 10000; ++i) { result += i * i; // 一些函数调用 @@ -47,7 +53,11 @@ TEST(FullFlowTest, GperftoolsGeneratesValidProfile) { } // Keep sampling real work rather than idling: see burnCPUFor(). - burnCPUFor(500); + const uint64_t burned = burnCPUFor(500); + // Used, not merely set: without an observation the loop can be elided, and both + // this and `result` trip -Wunused-but-set-variable under -Werror. + EXPECT_GT(burned, 0u); + EXPECT_GT(result, 0); // 停止 profiler ProfilerStop(); @@ -189,7 +199,7 @@ TEST(FullFlowTest, GetRawCPUProfile) { // from executing threads, so a 1s window with nothing to sample returns an // empty profile by design. The workload used to be absent here, which made // the non-empty assertion below depend on machine load. - std::thread burner{[]() { burnCPUFor(1200); }}; + std::thread burner{[]() { EXPECT_GT(burnCPUFor(1200), 0u); }}; // 获取原始 profile 数据（采样 1 秒） std::string profile_data = profiler.getRawCPUProfile(1); diff --git a/tests/test_logger.cpp b/tests/test_logger.cpp index ed82b00..d190579 100644 --- a/tests/test_logger.cpp +++ b/tests/test_logger.cpp @@ -166,10 +166,14 @@ TEST_F(LoggerTest, ThreadSafety) { std::vector threads; for (int t = 0; t < num_threads; ++t) { - threads.emplace_back([this, t, messages_per_thread]() { - for (int i = 0; i < messages_per_thread; ++i) { - mock_sink_->log(LogLevel::Info, "thread_test.cpp", t * messages_per_thread + i, "threadFunc", - ("Thread " + std::to_string(t) + " message " + std::to_string(i)).c_str()); + // The capture is const-foldable, so it is not captured; and the loop + // variable is "msg" rather than "i" because an "i" parameter would shadow + // the captured messages_per_thread, leaving it unused inside the lambda + // (clang rejects that under -Wunused-lambda-capture with -Werror). + threads.emplace_back([this, t]() { + for (int msg = 0; msg < messages_per_thread; ++msg) { + mock_sink_->log(LogLevel::Info, "thread_test.cpp", t * messages_per_thread + msg, "threadFunc", + ("Thread " + std::to_string(t) + " message " + std::to_string(msg)).c_str()); } }); } diff --git a/tests/test_web_layer.cpp b/tests/test_web_layer.cpp new file mode 100644 index 0000000..be09dd3 --- /dev/null +++ b/tests/test_web_layer.cpp @@ -0,0 +1,212 @@ +/// @file test_web_layer.cpp +/// @brief End-to-end test of the Drogon adapter and the embedded web resources +/// +/// Why this exists: every other test target is core-only (no Drogon), so +/// `drogon_adapter.cpp` and `web_resources.cpp` were compiled under ASan but never +/// executed by anything except the example binary, which ctest does not run. The +/// sanitizer build therefore looked like it covered the web layer while nothing +/// actually ran. This test starts the real server, drives it over HTTP and shuts it +/// down, so route registration, request delivery (including the async path through +/// AsyncExecutor and queueInLoop), the embedded pages and the shutdown sequence all +/// execute under the running sanitizer configuration. +/// +/// Requests are issued with a raw socket rather than libcurl, to avoid adding a +/// dependency purely for tests. A detail that makes the assertions meaningful: the +/// response writing happens on a *different* thread, so a response that arrives at +/// all proves the cross-thread hand-off worked, not merely that a handler ran. + +#include "../include/profiler/drogon_adapter.h" +#include "../include/profiler_manager.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +/// An HTTP response reduced to what the assertions need. +struct HttpResponse { + int status = 0; + std::string body; + bool received = false; +}; + +/// Send one request over a fresh connection and read the whole reply. +/// +/// Deliberately minimal: HTTP/1.1, `Connection: close`, so the reply ends when the +/// peer closes and no content-length parsing is needed. +HttpResponse request(uint16_t port, const std::string& method, const std::string& target, + const std::string& body = {}) { + HttpResponse result; + + const int fd = ::socket(AF_INET, SOCK_STREAM, 0); + if (fd < 0) + return result; + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_port = htons(port); + inet_pton(AF_INET, "127.0.0.1", &addr.sin_addr); + if (::connect(fd, reinterpret_cast(&addr), sizeof(addr)) != 0) { + ::close(fd); + return result; + } + + std::string req = method + " " + target + " HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n"; + if (!body.empty()) + req += "Content-Length: " + std::to_string(body.size()) + "\r\n"; + req += "\r\n" + body; + if (::write(fd, req.data(), req.size()) < 0) { + ::close(fd); + return result; + } + + std::string raw; + char buf[4096]; + ssize_t n = 0; + while ((n = ::read(fd, buf, sizeof(buf))) > 0) + raw.append(buf, static_cast(n)); + ::close(fd); + + const auto header_end = raw.find("\r\n\r\n"); + if (header_end == std::string::npos) + return result; + + result.received = true; + const auto status_line = raw.substr(0, raw.find("\r\n")); + const auto first_space = status_line.find(' '); + if (first_space != std::string::npos) + result.status = std::atoi(status_line.c_str() + first_space + 1); + result.body = raw.substr(header_end + 4); + return result; +} + +/// A Drogon server with the profiler routes, on an ephemeral port. +/// +/// The server is started once per *suite*, not once per test. `drogon::app()` is a +/// process-global singleton whose lifecycle is bind-once: calling addListener() a +/// second time, after a previous run() has finished, segfaults inside +/// ListenerManager. Structuring this as per-test SetUp/TearDown looked natural and +/// crashed on the second test. +/// +/// Port 0 lets the OS pick a free port and getListeners() reports which one, so +/// parallel ctest runs cannot collide. +class WebLayerTest : public ::testing::Test { +protected: + static void SetUpTestSuite() { + profiler_ = new profiler::ProfilerManager(); + profiler::registerDrogonHandlers(*profiler_); + + drogon::app().addListener("127.0.0.1", 0); + server_ = std::async(std::launch::async, []() { drogon::app().run(); }); + + // Wait for the listener to be bound rather than sleeping a fixed amount. + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); + while (std::chrono::steady_clock::now() < deadline) { + const auto listeners = drogon::app().getListeners(); + if (!listeners.empty()) { + port_ = listeners.front().toPort(); + break; + } + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + } + if (port_ == 0) + throw std::runtime_error("server never bound a port"); + } + + static void TearDownTestSuite() { + if (server_.valid()) { + drogon::app().getLoop()->queueInLoop([]() { drogon::app().quit(); }); + server_.wait(); + } + delete profiler_; + profiler_ = nullptr; + } + + static uint16_t port_; + +private: + static profiler::ProfilerManager* profiler_; + static std::future server_; +}; + +uint16_t WebLayerTest::port_ = 0; +profiler::ProfilerManager* WebLayerTest::profiler_ = nullptr; +std::future WebLayerTest::server_; + +} // namespace + +TEST_F(WebLayerTest, ServesTheEmbeddedControlPanel) { + // Exercises WebResources::getIndexPage(): the page is embedded as a C++ string + // literal, so nothing else checks that it is well-formed or that the route + // wiring returns it. + const auto resp = request(port_, "GET", "/"); + + ASSERT_TRUE(resp.received); + EXPECT_EQ(resp.status, 200); + EXPECT_NE(resp.body.find(""), std::string::npos); + // The panel must reference the current API surface, not a removed path. + EXPECT_NE(resp.body.find("/api/pprof/"), std::string::npos); +} + +TEST_F(WebLayerTest, ServesStatusAsJson) { + const auto resp = request(port_, "GET", "/api/status"); + + ASSERT_TRUE(resp.received); + EXPECT_EQ(resp.status, 200); + EXPECT_NE(resp.body.find("\"cpu\""), std::string::npos); + EXPECT_NE(resp.body.find("\"heap\""), std::string::npos); +} + +TEST_F(WebLayerTest, ServesThreadStacksThroughTheSignalPath) { + const auto resp = request(port_, "GET", "/api/thread/stacks"); + + ASSERT_TRUE(resp.received); + EXPECT_EQ(resp.status, 200); + // The capture machinery ran, which means the signal handler and the shared + // stack buffer were exercised on a request path. + EXPECT_NE(resp.body.find("Thread Call Stacks"), std::string::npos); +} + +TEST_F(WebLayerTest, ServesAAnalysisRequestThroughTheAsyncExecutor) { + // The analysis endpoints are the ones that go through AsyncExecutor and come + // back via queueInLoop, i.e. the response is written from another thread. That + // makes this the case most likely to leak or race, and growth is used because + // it needs neither TCMALLOC_SAMPLE_PARAMETER nor a sampling window. + const auto resp = request(port_, "GET", "/api/pprof/growth?renderer=flamegraph"); + + ASSERT_TRUE(resp.received); + // Either a rendered graph or a reported failure, but the reply itself proves the + // cross-thread hand-off completed. + EXPECT_TRUE(resp.status == 200 || resp.status == 500) << "status: " << resp.status; + // Braces are required here, not stylistic: the EXPECT_TRUE above expands to an + // if/else, so an unbraced if would dangle and GCC rejects it under + // -Wall -Wextra -Werror (the warnings-check job). + if (resp.status == 200) { + EXPECT_NE(resp.body.find("