Add test demonstrating thread destruction bug
This commit is contained in:
@@ -217,6 +217,9 @@ struct Metric {
|
|||||||
static Counter create_counter_instance(
|
static Counter create_counter_instance(
|
||||||
Family<Counter> *family,
|
Family<Counter> *family,
|
||||||
const std::vector<std::pair<std::string, std::string>> &labels) {
|
const std::vector<std::pair<std::string, std::string>> &labels) {
|
||||||
|
// Force thread_local initialization
|
||||||
|
(void)thread_init;
|
||||||
|
|
||||||
std::unique_lock<std::mutex> _{mutex};
|
std::unique_lock<std::mutex> _{mutex};
|
||||||
LabelsKey key{labels};
|
LabelsKey key{labels};
|
||||||
|
|
||||||
@@ -262,6 +265,9 @@ struct Metric {
|
|||||||
static Histogram create_histogram_instance(
|
static Histogram create_histogram_instance(
|
||||||
Family<Histogram> *family,
|
Family<Histogram> *family,
|
||||||
const std::vector<std::pair<std::string, std::string>> &labels) {
|
const std::vector<std::pair<std::string, std::string>> &labels) {
|
||||||
|
// Force thread_local initialization
|
||||||
|
(void)thread_init;
|
||||||
|
|
||||||
std::unique_lock<std::mutex> _{mutex};
|
std::unique_lock<std::mutex> _{mutex};
|
||||||
LabelsKey key{labels};
|
LabelsKey key{labels};
|
||||||
auto &ptr =
|
auto &ptr =
|
||||||
|
|||||||
@@ -483,6 +483,113 @@ TEST_CASE("thread safety") {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
TEST_CASE("thread counter cleanup bug") {
|
||||||
|
SUBCASE(
|
||||||
|
"counter and histogram values should persist after thread destruction") {
|
||||||
|
auto counter_family = metric::create_counter(
|
||||||
|
"thread_cleanup_counter", "Counter for thread cleanup test");
|
||||||
|
auto histogram_family = metric::create_histogram(
|
||||||
|
"thread_cleanup_histogram", "Histogram for thread cleanup test",
|
||||||
|
metric::linear_buckets(0.0, 1.0, 5)); // buckets: 0, 1, 2, 3, 4
|
||||||
|
|
||||||
|
// Variables to collect actual values from worker thread
|
||||||
|
double counter_value_in_thread = 0;
|
||||||
|
double histogram_sum_in_thread = 0;
|
||||||
|
|
||||||
|
// Create thread that increments metrics and then exits
|
||||||
|
std::thread worker([&]() {
|
||||||
|
auto counter = counter_family.create({{"worker", "cleanup_test"}});
|
||||||
|
auto histogram = histogram_family.create({{"worker", "cleanup_test"}});
|
||||||
|
|
||||||
|
counter.inc(1.0);
|
||||||
|
histogram.observe(1.5); // Should contribute to sum
|
||||||
|
|
||||||
|
// Measure actual values from within the thread (before ThreadInit
|
||||||
|
// destructor runs)
|
||||||
|
ArenaAllocator thread_arena;
|
||||||
|
auto thread_output = metric::render(thread_arena);
|
||||||
|
|
||||||
|
for (const auto &line : thread_output) {
|
||||||
|
if (line.find("thread_cleanup_counter{worker=\"cleanup_test\"}") !=
|
||||||
|
std::string_view::npos) {
|
||||||
|
auto space_pos = line.rfind(' ');
|
||||||
|
if (space_pos != std::string_view::npos) {
|
||||||
|
auto value_str = line.substr(space_pos + 1);
|
||||||
|
if (value_str.back() == '\n') {
|
||||||
|
value_str.remove_suffix(1);
|
||||||
|
}
|
||||||
|
counter_value_in_thread = std::stod(std::string(value_str));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if (line.find(
|
||||||
|
"thread_cleanup_histogram_sum{worker=\"cleanup_test\"}") !=
|
||||||
|
std::string_view::npos) {
|
||||||
|
auto space_pos = line.rfind(' ');
|
||||||
|
if (space_pos != std::string_view::npos) {
|
||||||
|
auto value_str = line.substr(space_pos + 1);
|
||||||
|
if (value_str.back() == '\n') {
|
||||||
|
value_str.remove_suffix(1);
|
||||||
|
}
|
||||||
|
histogram_sum_in_thread = std::stod(std::string(value_str));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// Wait for thread to complete and destroy (triggering ThreadInit
|
||||||
|
// destructor)
|
||||||
|
worker.join();
|
||||||
|
|
||||||
|
// Measure values after thread cleanup
|
||||||
|
ArenaAllocator arena;
|
||||||
|
auto output = metric::render(arena);
|
||||||
|
|
||||||
|
double counter_value_after = 0;
|
||||||
|
double histogram_sum_after = 0;
|
||||||
|
|
||||||
|
for (const auto &line : output) {
|
||||||
|
if (line.find("thread_cleanup_counter{worker=\"cleanup_test\"}") !=
|
||||||
|
std::string_view::npos) {
|
||||||
|
auto space_pos = line.rfind(' ');
|
||||||
|
if (space_pos != std::string_view::npos) {
|
||||||
|
auto value_str = line.substr(space_pos + 1);
|
||||||
|
if (value_str.back() == '\n') {
|
||||||
|
value_str.remove_suffix(1);
|
||||||
|
}
|
||||||
|
counter_value_after = std::stod(std::string(value_str));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if (line.find("thread_cleanup_histogram_sum{worker=\"cleanup_test\"}") !=
|
||||||
|
std::string_view::npos) {
|
||||||
|
auto space_pos = line.rfind(' ');
|
||||||
|
if (space_pos != std::string_view::npos) {
|
||||||
|
auto value_str = line.substr(space_pos + 1);
|
||||||
|
if (value_str.back() == '\n') {
|
||||||
|
value_str.remove_suffix(1);
|
||||||
|
}
|
||||||
|
histogram_sum_after = std::stod(std::string(value_str));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Values should have been captured correctly within the thread
|
||||||
|
CHECK(counter_value_in_thread == 1.0);
|
||||||
|
CHECK(histogram_sum_in_thread == 1.5);
|
||||||
|
|
||||||
|
// The bug: These values should persist after thread cleanup but will be
|
||||||
|
// lost because ThreadInit destructor erases per-thread state without
|
||||||
|
// accumulating values
|
||||||
|
CHECK(counter_value_after == 1.0);
|
||||||
|
CHECK(histogram_sum_after == 1.5);
|
||||||
|
|
||||||
|
// The bug: After thread destruction, the counter and histogram values are
|
||||||
|
// lost because ThreadInit::~ThreadInit() calls
|
||||||
|
// family->perThreadState.erase(thread_id) without accumulating the values
|
||||||
|
// into global storage first. This causes counter values to "go backwards"
|
||||||
|
// when threads are destroyed, violating the monotonic property of counters.
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
TEST_CASE("error conditions") {
|
TEST_CASE("error conditions") {
|
||||||
SUBCASE("counter negative increment") {
|
SUBCASE("counter negative increment") {
|
||||||
auto counter_family = metric::create_counter("error_counter", "Error test");
|
auto counter_family = metric::create_counter("error_counter", "Error test");
|
||||||
|
|||||||
Reference in New Issue
Block a user