Fix Family::get_or_create

The method should not overwrite an existing key after it obtains the write lock. Consider the following scenario with `Family<LabelSet, Gauge>` used by threads A and B: 1. A and B both call `family.get_or_insert(&label_set)` 2. A and B both acquire read lock and finds no key 3. A gets write lock and inserts a new gauge with value 0 4. A increments returned gauge to 1 5. B gets write lock and inserts a new gauge with value 0 6. B increments returned gauge to 1 7. A decrements gauge back to 0 8. B decrements gauge which now overflows to `u64::MAX` Signed-off-by: Anthony Ramine <[email protected]>
prometheus · Oct 13, 2022 · 290369f · 290369f
1 parent de81e7b
commit 290369f
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,9 +18,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Move`Encode` trait from `prometheus_client::encoding::text` to `prometheus_client::encoding`. See [PR 83].
 
+### Fixed
+
+- Fix race condition in `Family::get_or_create`. See [PR 102].
+
 [PR 83]: https://github.com/prometheus/client_rust/pull/83
 [PR 85]: https://github.com/prometheus/client_rust/pull/85
 [PR 96]: https://github.com/prometheus/client_rust/pull/96
+[PR 102]: https://github.com/prometheus/client_rust/pull/102
 
 ## [0.18.0]
 

diff --git a/src/metrics/family.rs b/src/metrics/family.rs
@@ -3,7 +3,7 @@
 //! See [`Family`] for details.
 
 use super::{MetricType, TypedMetric};
-use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
+use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard, RwLockWriteGuard};
 use std::collections::HashMap;
 use std::sync::Arc;
 
@@ -223,11 +223,14 @@ impl<S: Clone + std::hash::Hash + Eq, M, C: MetricConstructor<M>> Family<S, M, C
         }
 
         let mut write_guard = self.metrics.write();
-        write_guard.insert(label_set.clone(), self.constructor.new_metric());
 
-        drop(write_guard);
+        write_guard
+            .entry(label_set.clone())
+            .or_insert_with(|| self.constructor.new_metric());
 
-        RwLockReadGuard::map(self.metrics.read(), |metrics| {
+        let read_guard = RwLockWriteGuard::downgrade(write_guard);
+
+        RwLockReadGuard::map(read_guard, |metrics| {
             metrics
                 .get(label_set)
                 .expect("Metric to exist after creating it.")