Skip to content

Commit

Permalink
*: Move to parking_lot and thus make owning_ref obsolete
Browse files Browse the repository at this point in the history
Before `proemtheus-client` would use the `owning_ref` crate to map the target
of a `std::sync::RwLockReadGuard`. `owning_ref` has multiple unsoundness
issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of
replacing `owning_ref` with a similar crate, we switch to locking via
`parking_lot` which supports the above mapping natively.

Signed-off-by: Max Inden <[email protected]>
  • Loading branch information
mxinden committed Aug 6, 2022
1 parent 6b7a345 commit ee64d73
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 36 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.18.0] - unreleased

### Changed

- Use `parking_lot` instead of `std::sync::*`.

Before `proemtheus-client` would use the `owning_ref` crate to map the target
of a `std::sync::RwLockReadGuard`. `owning_ref` has multiple unsoundness
issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of
replacing `owning_ref` with a similar crate, we switch to locking via
`parking_lot` which supports the above mapping natively.

## [0.17.0]

### Changed
Expand Down
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "prometheus-client"
version = "0.17.0"
version = "0.18.0"
authors = ["Max Inden <[email protected]>"]
edition = "2021"
description = "Open Metrics client library allowing users to natively instrument applications."
Expand All @@ -16,7 +16,7 @@ members = ["derive-text-encode"]
[dependencies]
dtoa = "1.0"
itoa = "1.0"
owning_ref = "0.4"
parking_lot = "0.12"
prometheus-client-derive-text-encode = { version = "0.3.0", path = "derive-text-encode" }

[dev-dependencies]
Expand Down
2 changes: 1 addition & 1 deletion src/encoding/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ where
{
fn encode(&self, encoder: Encoder) -> Result<(), std::io::Error> {
let (value, exemplar) = self.get();
encode_counter_with_maybe_exemplar(value, exemplar.as_ref().as_ref(), encoder)
encode_counter_with_maybe_exemplar(value, exemplar.as_ref(), encoder)
}

fn metric_type(&self) -> MetricType {
Expand Down
24 changes: 10 additions & 14 deletions src/metrics/exemplar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
use super::counter::{self, Counter};
use super::histogram::Histogram;
use owning_ref::OwningRef;
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
use std::collections::HashMap;
#[cfg(any(target_arch = "mips", target_arch = "powerpc"))]
use std::sync::atomic::AtomicU32;
#[cfg(not(any(target_arch = "mips", target_arch = "powerpc")))]
use std::sync::atomic::AtomicU64;
use std::sync::{Arc, RwLock, RwLockReadGuard};
use std::sync::Arc;

#[derive(Debug)]
pub struct Exemplar<S, V> {
Expand Down Expand Up @@ -74,7 +74,7 @@ impl<S, N: Clone, A: counter::Atomic<N>> CounterWithExemplar<S, N, A> {
/// Increase the [`CounterWithExemplar`] by `v`, updating the [`Exemplar`]
/// if a label set is provided, returning the previous value.
pub fn inc_by(&self, v: N, label_set: Option<S>) -> N {
let mut inner = self.inner.write().expect("Lock not to be poisoned.");
let mut inner = self.inner.write();

inner.exemplar = label_set.map(|label_set| Exemplar {
label_set,
Expand All @@ -86,10 +86,10 @@ impl<S, N: Clone, A: counter::Atomic<N>> CounterWithExemplar<S, N, A> {

/// Get the current value of the [`CounterWithExemplar`] as well as its
/// [`Exemplar`] if any.
pub fn get(&self) -> (N, RwLockGuardedCounterWithExemplar<S, N, A>) {
let inner = self.inner.read().expect("Lock not to be poisoned.");
pub fn get(&self) -> (N, MappedRwLockReadGuard<Option<Exemplar<S, N>>>) {
let inner = self.inner.read();
let value = inner.counter.get();
let exemplar = OwningRef::new(inner).map(|inner| &inner.exemplar);
let exemplar = RwLockReadGuard::map(inner, |inner| &inner.exemplar);
(value, exemplar)
}

Expand All @@ -101,15 +101,11 @@ impl<S, N: Clone, A: counter::Atomic<N>> CounterWithExemplar<S, N, A> {
/// The caller of this function has to uphold the property of an Open
/// Metrics counter namely that the value is monotonically increasing, i.e.
/// either stays the same or increases.
pub fn inner(&self) -> OwningRef<RwLockReadGuard<CounterWithExemplarInner<S, N, A>>, A> {
OwningRef::new(self.inner.read().expect("Lock not to be poisoned."))
.map(|inner| inner.counter.inner())
pub fn inner(&self) -> MappedRwLockReadGuard<A> {
RwLockReadGuard::map(self.inner.read(), |inner| inner.counter.inner())
}
}

type RwLockGuardedCounterWithExemplar<'a, S, N, A> =
OwningRef<RwLockReadGuard<'a, CounterWithExemplarInner<S, N, A>>, Option<Exemplar<S, N>>>;

/////////////////////////////////////////////////////////////////////////////////
// Histogram

Expand Down Expand Up @@ -153,7 +149,7 @@ impl<S> HistogramWithExemplars<S> {
}

pub fn observe(&self, v: f64, label_set: Option<S>) {
let mut inner = self.inner.write().expect("Lock not to be poisoned.");
let mut inner = self.inner.write();
let bucket = inner.histogram.observe_and_bucket(v);
if let (Some(bucket), Some(label_set)) = (bucket, label_set) {
inner.exemplars.insert(
Expand All @@ -167,6 +163,6 @@ impl<S> HistogramWithExemplars<S> {
}

pub(crate) fn inner(&self) -> RwLockReadGuard<HistogramWithExemplarsInner<S>> {
self.inner.read().expect("Lock not to be poisoned.")
self.inner.read()
}
}
16 changes: 7 additions & 9 deletions src/metrics/family.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
//! See [`Family`] for details.
use super::{MetricType, TypedMetric};
use owning_ref::OwningRef;
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
use std::collections::HashMap;
use std::sync::{Arc, RwLock, RwLockReadGuard};
use std::sync::Arc;

/// Representation of the OpenMetrics *MetricFamily* data type.
///
Expand Down Expand Up @@ -214,29 +214,27 @@ impl<S: Clone + std::hash::Hash + Eq, M, C: MetricConstructor<M>> Family<S, M, C
/// // calls.
/// family.get_or_create(&vec![("method".to_owned(), "GET".to_owned())]).inc();
/// ```
pub fn get_or_create(&self, label_set: &S) -> OwningRef<RwLockReadGuard<HashMap<S, M>>, M> {
let read_guard = self.metrics.read().expect("Lock not to be poisoned.");
pub fn get_or_create(&self, label_set: &S) -> MappedRwLockReadGuard<M> {
if let Ok(metric) =
OwningRef::new(read_guard).try_map(|metrics| metrics.get(label_set).ok_or(()))
RwLockReadGuard::try_map(self.metrics.read(), |metrics| metrics.get(label_set))
{
return metric;
}

let mut write_guard = self.metrics.write().unwrap();
let mut write_guard = self.metrics.write();
write_guard.insert(label_set.clone(), self.constructor.new_metric());

drop(write_guard);

let read_guard = self.metrics.read().unwrap();
OwningRef::new(read_guard).map(|metrics| {
RwLockReadGuard::map(self.metrics.read(), |metrics| {
metrics
.get(label_set)
.expect("Metric to exist after creating it.")
})
}

pub(crate) fn read(&self) -> RwLockReadGuard<HashMap<S, M>> {
self.metrics.read().unwrap()
self.metrics.read()
}
}

Expand Down
18 changes: 8 additions & 10 deletions src/metrics/histogram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
//! See [`Histogram`] for details.
use super::{MetricType, TypedMetric};
use owning_ref::OwningRef;
use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
use std::iter::{self, once};
use std::sync::{Arc, Mutex, MutexGuard};
use std::sync::Arc;

/// Open Metrics [`Histogram`] to measure distributions of discrete events.
///
Expand Down Expand Up @@ -33,7 +33,7 @@ use std::sync::{Arc, Mutex, MutexGuard};
// https://github.com/tikv/rust-prometheus/pull/314.
#[derive(Debug)]
pub struct Histogram {
inner: Arc<Mutex<Inner>>,
inner: Arc<RwLock<Inner>>,
}

impl Clone for Histogram {
Expand All @@ -56,7 +56,7 @@ pub(crate) struct Inner {
impl Histogram {
pub fn new(buckets: impl Iterator<Item = f64>) -> Self {
Self {
inner: Arc::new(Mutex::new(Inner {
inner: Arc::new(RwLock::new(Inner {
sum: Default::default(),
count: Default::default(),
buckets: buckets
Expand All @@ -78,7 +78,7 @@ impl Histogram {
/// Needed in
/// [`HistogramWithExemplars`](crate::metrics::exemplar::HistogramWithExemplars).
pub(crate) fn observe_and_bucket(&self, v: f64) -> Option<usize> {
let mut inner = self.inner.lock().unwrap();
let mut inner = self.inner.write();
inner.sum += v;
inner.count += 1;

Expand All @@ -97,17 +97,15 @@ impl Histogram {
}
}

pub(crate) fn get(&self) -> (f64, u64, MutexGuardedBuckets) {
let inner = self.inner.lock().unwrap();
pub(crate) fn get(&self) -> (f64, u64, MappedRwLockReadGuard<Vec<(f64, u64)>>) {
let inner = self.inner.read();
let sum = inner.sum;
let count = inner.count;
let buckets = OwningRef::new(inner).map(|inner| &inner.buckets);
let buckets = RwLockReadGuard::map(inner, |inner| &inner.buckets);
(sum, count, buckets)
}
}

pub(crate) type MutexGuardedBuckets<'a> = OwningRef<MutexGuard<'a, Inner>, Vec<(f64, u64)>>;

impl TypedMetric for Histogram {
const TYPE: MetricType = MetricType::Histogram;
}
Expand Down

0 comments on commit ee64d73

Please sign in to comment.