From 750c451e96e671e5e69de0b4af7a252b8d25a0f0 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 12 Mar 2024 08:14:08 -0700 Subject: [PATCH] [fake stats plugin] add locking to avoid tsan failures (#36093) A couple of recent flakes: - https://btx-internal.corp.google.com/invocations/849676bf-0e4e-4ccf-9c17-21c19d6c0da5/targets/%2F%2Ftest%2Fcpp%2Fend2end:rls_end2end_test@poller%3Depoll1;config=b8fe4b9e3f3b31af34590e00748b2a7a095cedd3b431113a6063d7780bb24eca/log - https://btx-internal.corp.google.com/invocations/9515330c-a165-4ab5-81a1-3285d8a16bbb/targets/%2F%2Ftest%2Fcpp%2Fend2end%2Fxds:xds_wrr_end2end_test@experiment%3Dno_pick_first_happy_eyeballs;config=27047b7cfce2ec428f95a56e3bcafdc2cb72a602f6a43af1336b4c235e772213/log Closes #36093 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36093 from markdroth:fake_stats_plugin_tsan_fix f504330684989bd0acbc7c0d0169b5e9fac5244d PiperOrigin-RevId: 615045684 --- test/core/util/fake_stats_plugin.h | 58 +++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/test/core/util/fake_stats_plugin.h b/test/core/util/fake_stats_plugin.h index 04268ca694e..e6645f1df72 100644 --- a/test/core/util/fake_stats_plugin.h +++ b/test/core/util/fake_stats_plugin.h @@ -220,7 +220,8 @@ class FakeStatsPlugin : public StatsPlugin { return; } switch (descriptor.instrument_type) { - case GlobalInstrumentsRegistry::InstrumentType::kCounter: + case GlobalInstrumentsRegistry::InstrumentType::kCounter: { + MutexLock lock(&mu_); if (descriptor.value_type == GlobalInstrumentsRegistry::ValueType::kUInt64) { uint64_counters_.emplace(descriptor.index, descriptor); @@ -228,7 +229,9 @@ class FakeStatsPlugin : public StatsPlugin { double_counters_.emplace(descriptor.index, descriptor); } break; - case GlobalInstrumentsRegistry::InstrumentType::kHistogram: + } + case GlobalInstrumentsRegistry::InstrumentType::kHistogram: { + MutexLock lock(&mu_); if (descriptor.value_type == GlobalInstrumentsRegistry::ValueType::kUInt64) { uint64_histograms_.emplace(descriptor.index, descriptor); @@ -236,7 +239,9 @@ class FakeStatsPlugin : public StatsPlugin { double_histograms_.emplace(descriptor.index, descriptor); } break; - case GlobalInstrumentsRegistry::InstrumentType::kGauge: + } + case GlobalInstrumentsRegistry::InstrumentType::kGauge: { + MutexLock lock(&mu_); if (descriptor.value_type == GlobalInstrumentsRegistry::ValueType::kInt64) { int64_gauges_.emplace(descriptor.index, descriptor); @@ -244,7 +249,9 @@ class FakeStatsPlugin : public StatsPlugin { double_gauges_.emplace(descriptor.index, descriptor); } break; - case GlobalInstrumentsRegistry::InstrumentType::kCallbackGauge: + } + case GlobalInstrumentsRegistry::InstrumentType::kCallbackGauge: { + MutexLock lock(&callback_mu_); if (descriptor.value_type == GlobalInstrumentsRegistry::ValueType::kInt64) { int64_callback_gauges_.emplace(descriptor.index, descriptor); @@ -252,6 +259,7 @@ class FakeStatsPlugin : public StatsPlugin { double_callback_gauges_.emplace(descriptor.index, descriptor); } break; + } default: Crash("unknown instrument type"); } @@ -286,6 +294,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&mu_); auto iter = uint64_counters_.find(handle.index); if (iter == uint64_counters_.end()) return; iter->second.Add(value, label_values, optional_values); @@ -300,6 +309,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&mu_); auto iter = double_counters_.find(handle.index); if (iter == double_counters_.end()) return; iter->second.Add(value, label_values, optional_values); @@ -314,6 +324,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&mu_); auto iter = uint64_histograms_.find(handle.index); if (iter == uint64_histograms_.end()) return; iter->second.Record(value, label_values, optional_values); @@ -328,6 +339,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&mu_); auto iter = double_histograms_.find(handle.index); if (iter == double_histograms_.end()) return; iter->second.Record(value, label_values, optional_values); @@ -341,6 +353,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&mu_); auto iter = int64_gauges_.find(handle.index); if (iter == int64_gauges_.end()) return; iter->second.Set(value, label_values, optional_values); @@ -354,6 +367,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&mu_); auto iter = double_gauges_.find(handle.index); if (iter == double_gauges_.end()) return; iter->second.Set(value, label_values, optional_values); @@ -372,6 +386,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalUInt64CounterHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&mu_); auto iter = uint64_counters_.find(handle.index); if (iter == uint64_counters_.end()) { return absl::nullopt; @@ -382,6 +397,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalDoubleCounterHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&mu_); auto iter = double_counters_.find(handle.index); if (iter == double_counters_.end()) { return absl::nullopt; @@ -392,6 +408,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalUInt64HistogramHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&mu_); auto iter = uint64_histograms_.find(handle.index); if (iter == uint64_histograms_.end()) { return absl::nullopt; @@ -402,6 +419,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalDoubleHistogramHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&mu_); auto iter = double_histograms_.find(handle.index); if (iter == double_histograms_.end()) { return absl::nullopt; @@ -412,6 +430,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalInt64GaugeHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&mu_); auto iter = int64_gauges_.find(handle.index); if (iter == int64_gauges_.end()) { return absl::nullopt; @@ -422,6 +441,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalDoubleGaugeHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&mu_); auto iter = double_gauges_.find(handle.index); if (iter == double_gauges_.end()) { return absl::nullopt; @@ -440,6 +460,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalCallbackInt64GaugeHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&callback_mu_); auto iter = int64_callback_gauges_.find(handle.index); if (iter == int64_callback_gauges_.end()) { return absl::nullopt; @@ -450,6 +471,7 @@ class FakeStatsPlugin : public StatsPlugin { GlobalInstrumentsRegistry::GlobalCallbackDoubleGaugeHandle handle, absl::Span label_values, absl::Span optional_values) { + MutexLock lock(&callback_mu_); auto iter = double_callback_gauges_.find(handle.index); if (iter == double_callback_gauges_.end()) { return absl::nullopt; @@ -473,6 +495,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&plugin_.callback_mu_); auto iter = plugin_.int64_callback_gauges_.find(handle.index); if (iter == plugin_.int64_callback_gauges_.end()) return; iter->second.Set(value, label_values, optional_values); @@ -489,6 +512,7 @@ class FakeStatsPlugin : public StatsPlugin { this, handle.index, value, absl::StrJoin(label_values, ", ").c_str(), absl::StrJoin(optional_values, ", ").c_str()); + MutexLock lock(&plugin_.callback_mu_); auto iter = plugin_.double_callback_gauges_.find(handle.index); if (iter == plugin_.double_callback_gauges_.end()) return; iter->second.Set(value, label_values, optional_values); @@ -621,14 +645,24 @@ class FakeStatsPlugin : public StatsPlugin { absl::AnyInvocable channel_filter_; // Instruments. - absl::flat_hash_map> uint64_counters_; - absl::flat_hash_map> double_counters_; - absl::flat_hash_map> uint64_histograms_; - absl::flat_hash_map> double_histograms_; - absl::flat_hash_map> int64_gauges_; - absl::flat_hash_map> double_gauges_; - absl::flat_hash_map> int64_callback_gauges_; - absl::flat_hash_map> double_callback_gauges_; + Mutex mu_; + absl::flat_hash_map> uint64_counters_ + ABSL_GUARDED_BY(&mu_); + absl::flat_hash_map> double_counters_ + ABSL_GUARDED_BY(&mu_); + absl::flat_hash_map> uint64_histograms_ + ABSL_GUARDED_BY(&mu_); + absl::flat_hash_map> double_histograms_ + ABSL_GUARDED_BY(&mu_); + absl::flat_hash_map> int64_gauges_ + ABSL_GUARDED_BY(&mu_); + absl::flat_hash_map> double_gauges_ + ABSL_GUARDED_BY(&mu_); + Mutex callback_mu_; + absl::flat_hash_map> int64_callback_gauges_ + ABSL_GUARDED_BY(&callback_mu_); + absl::flat_hash_map> double_callback_gauges_ + ABSL_GUARDED_BY(&callback_mu_); std::set callbacks_; };