Skip to content

Commit

Permalink
Drop incorrect use of reentrant locks (#1076)
Browse files Browse the repository at this point in the history
This fixes a correctness bug introduced in
0014e97 resulting in lost updates
during some scenarios.

The code being locked is not reentrant safe. It's preferable to deadlock
in these situations instead of silently loosing updates for example.

Signed-off-by: Przemysław Suliga <mail@suligap.net>
  • Loading branch information
suligap authored and csmarchbanks committed Dec 3, 2024
1 parent 3b183b4 commit 591e91f
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 18 deletions.
8 changes: 4 additions & 4 deletions prometheus_client/metrics.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import os
from threading import RLock
from threading import Lock
import time
import types
from typing import (
Expand Down Expand Up @@ -144,7 +144,7 @@ def __init__(self: T,

if self._is_parent():
# Prepare the fields needed for child metrics.
self._lock = RLock()
self._lock = Lock()
self._metrics: Dict[Sequence[str], T] = {}

if self._is_observable():
Expand Down Expand Up @@ -697,7 +697,7 @@ class Info(MetricWrapperBase):

def _metric_init(self):
self._labelname_set = set(self._labelnames)
self._lock = RLock()
self._lock = Lock()
self._value = {}

def info(self, val: Dict[str, str]) -> None:
Expand Down Expand Up @@ -759,7 +759,7 @@ def __init__(self,

def _metric_init(self) -> None:
self._value = 0
self._lock = RLock()
self._lock = Lock()

def state(self, state: str) -> None:
"""Set enum metric state."""
Expand Down
4 changes: 2 additions & 2 deletions prometheus_client/registry.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from abc import ABC, abstractmethod
import copy
from threading import RLock
from threading import Lock
from typing import Dict, Iterable, List, Optional

from .metrics_core import Metric
Expand Down Expand Up @@ -30,7 +30,7 @@ def __init__(self, auto_describe: bool = False, target_info: Optional[Dict[str,
self._collector_to_names: Dict[Collector, List[str]] = {}
self._names_to_collectors: Dict[str, Collector] = {}
self._auto_describe = auto_describe
self._lock = RLock()
self._lock = Lock()
self._target_info: Optional[Dict[str, str]] = {}
self.set_target_info(target_info)

Expand Down
6 changes: 3 additions & 3 deletions prometheus_client/values.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import os
from threading import RLock
from threading import Lock
import warnings

from .mmap_dict import mmap_key, MmapedDict
Expand All @@ -13,7 +13,7 @@ class MutexValue:
def __init__(self, typ, metric_name, name, labelnames, labelvalues, help_text, **kwargs):
self._value = 0.0
self._exemplar = None
self._lock = RLock()
self._lock = Lock()

def inc(self, amount):
with self._lock:
Expand Down Expand Up @@ -50,7 +50,7 @@ def MultiProcessValue(process_identifier=os.getpid):
# Use a single global lock when in multi-processing mode
# as we presume this means there is no threading going on.
# This avoids the need to also have mutexes in __MmapDict.
lock = RLock()
lock = Lock()

class MmapedValue:
"""A float protected by a mutex backed by a per-process mmaped file."""
Expand Down
10 changes: 1 addition & 9 deletions tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,6 @@
from prometheus_client.metrics import _get_use_created


def is_locked(lock):
"Tries to obtain a lock, returns True on success, False on failure."
locked = lock.acquire(blocking=False)
if locked:
lock.release()
return not locked


def assert_not_observable(fn, *args, **kwargs):
"""
Assert that a function call falls with a ValueError exception containing
Expand Down Expand Up @@ -971,7 +963,7 @@ def test_restricted_registry_does_not_yield_while_locked(self):
m = Metric('target', 'Target metadata', 'info')
m.samples = [Sample('target_info', {'foo': 'bar'}, 1)]
for _ in registry.restricted_registry(['target_info', 's_sum']).collect():
self.assertFalse(is_locked(registry._lock))
self.assertFalse(registry._lock.locked())


if __name__ == '__main__':
Expand Down

0 comments on commit 591e91f

Please sign in to comment.