Repository: prometheus/client_python
Pull request #371 (2019-02-10 21:41:59)

Deadlock on gcCollector
Hello!
Regading https://github.com/prometheus/client_python/issues/363

We have the same problem with asyncio using (aiohttp, single process, single thread)

As we cannot predict when `gc.collect()` will be executed (with `gc.enable`), it is not possible to use something that has locks inside the `gc.collect` callbacks. So, YES, we neet to reconsidered how the GC collector works.
 
One way it is remove GC Collector  :sunglasses:  . 
Another way in this PR - Collect metrics in simle structures witout locks and represent it in 'collect' method.
Therd way is to use `gc.stats` (since python 3.4) with Gauge (`GaugeMetricFamily`) metrics at `collect`.
Or as @megabotan said, just remove lock in `get` methof of `MutexValue`

brian-brazil: I still think we'd be better off using gc.stats.
Krukov: @brian-brazil Agree with you. I will prepare PR asap
Krukov: @brian-brazil I remade PR to using `gc.stats`. Can you review it? 

Patch:
diff --git a/prometheus_client/gc_collector.py b/prometheus_client/gc_collector.py
index 8d7d740..de400d9 100644
--- a/prometheus_client/gc_collector.py
+++ b/prometheus_client/gc_collector.py
@@ -3,79 +3,44 @@
 from __future__ import unicode_literals
 
 import gc
-import os
-import time
 
-from .metrics import Histogram
+from .metrics_core import CounterMetricFamily
 from .registry import REGISTRY
 
 
 class GCCollector(object):
     """Collector for Garbage collection statistics."""
 
-    def __init__(self, registry=REGISTRY, gc=gc):
-        # To work around the deadlock issue described in
-        # https://github.com/prometheus/client_python/issues/322,
-        # the GC collector is always disabled in multiprocess mode.
-        if 'prometheus_multiproc_dir' in os.environ:
+    def __init__(self, registry=REGISTRY):
+        if not hasattr(gc, 'get_stats'):
             return
+        registry.register(self)
 
-        if not hasattr(gc, 'callbacks'):
-            return
-
-        collected = Histogram(
-            'python_gc_collected_objects',
+    def collect(self):
+        collected = CounterMetricFamily(
+            'python_gc_objects_collected',
             'Objects collected during gc',
-            ['generation'],
-            buckets=[500, 1000, 5000, 10000, 50000],
-            registry=registry
+            labels=['generation'],
         )
-
-        uncollectable = Histogram(
-            'python_gc_uncollectable_objects',
+        uncollectable = CounterMetricFamily(
+            'python_gc_objects_uncollectable',
             'Uncollectable object found during GC',
-            ['generation'],
-            buckets=[500, 1000, 5000, 10000, 50000],
-            registry=registry
+            labels=['generation'],
         )
 
-        latency = Histogram(
-            'python_gc_duration_seconds',
-            'Time spent in garbage collection',
-            ['generation'],
-            registry=registry
+        collections = CounterMetricFamily(
+            'python_gc_collections',
+            'Number of times this generation was collected',
+            labels=['generation'],
         )
 
-        times = {}
-
-        # Avoid _cb() being called re-entrantly
-        # by setting this flag and clearing it once
-        # the callback operation is complete.
-        # See https://github.com/prometheus/client_python/issues/322#issuecomment-438021132
-        self.gc_cb_active = False
-
-        def _cb(phase, info):
-            try:
-                if self.gc_cb_active:
-                    return
-                self.gc_cb_active = True
-
-                gen = info['generation']
-
-                if phase == 'start':
-                    times[gen] = time.time()
-
-                if phase == 'stop':
-                    delta = time.time() - times[gen]
-                    latency.labels(gen).observe(delta)
-                    if 'collected' in info:
-                        collected.labels(gen).observe(info['collected'])
-                    if 'uncollectable' in info:
-                        uncollectable.labels(gen).observe(info['uncollectable'])
-            finally:
-                self.gc_cb_active = False
+        for generation, stat in enumerate(gc.get_stats()):
+            generation = str(generation)
+            collected.add_metric([generation], value=stat['collected'])
+            uncollectable.add_metric([generation], value=stat['uncollectable'])
+            collections.add_metric([generation], value=stat['collections'])
 
-        gc.callbacks.append(_cb)
+        return [collected, uncollectable, collections]
 
 
 GC_COLLECTOR = GCCollector()


Test patch:
diff --git a/tests/test_gc_collector.py b/tests/test_gc_collector.py
index a317d60..98f3ccd 100644
--- a/tests/test_gc_collector.py
+++ b/tests/test_gc_collector.py
@@ -1,54 +1,62 @@
 from __future__ import unicode_literals
 
-import unittest
+import gc
+import sys
+
+if sys.version_info < (2, 7):
+    # We need the skip decorators from unittest2 on Python 2.6.
+    import unittest2 as unittest
+else:
+    import unittest
 
 from prometheus_client import CollectorRegistry, GCCollector
 
 
+@unittest.skipIf(sys.version_info < (3, 4), "Test requires Python 3.4 +")
 class TestGCCollector(unittest.TestCase):
     def setUp(self):
+        gc.disable()
+        gc.collect()
         self.registry = CollectorRegistry()
-        self.gc = _MockGC()
 
     def test_working(self):
-        collector = GCCollector(registry=self.registry, gc=self.gc)
-        self.gc.start_gc({'generation': 0})
-        self.gc.stop_gc({'generation': 0, 'collected': 10, 'uncollectable': 2})
-
-        self.assertEqual(1,
-                         self.registry.get_sample_value(
-                             'python_gc_duration_seconds_count',
-                             labels={"generation": "0"}))
-
-        self.assertEqual(1,
-                         self.registry.get_sample_value(
-                             'python_gc_collected_objects_count',
-                             labels={"generation": "0"}))
-
-        self.assertEqual(1,
-                         self.registry.get_sample_value(
-                             'python_gc_uncollectable_objects_count',
-                             labels={"generation": "0"}))
-
-        self.assertEqual(10,
-                         self.registry.get_sample_value(
-                             'python_gc_collected_objects_sum',
-                             labels={"generation": "0"}))
 
-        self.assertEqual(2,
+        GCCollector(registry=self.registry)
+        self.registry.collect()
+        before = self.registry.get_sample_value('python_gc_objects_collected_total',
+                                                labels={"generation": "0"})
+
+        #  add targets for gc
+        a = []
+        a.append(a)
+        del a
+        b = []
+        b.append(b)
+        del b
+
+        gc.collect(0)
+        self.registry.collect()
+
+        after = self.registry.get_sample_value('python_gc_objects_collected_total',
+                                               labels={"generation": "0"})
+        self.assertEqual(2, after - before)
+        self.assertEqual(0,
                          self.registry.get_sample_value(
-                             'python_gc_uncollectable_objects_sum',
+                             'python_gc_objects_uncollectable_total',
                              labels={"generation": "0"}))
 
+    def test_empty(self):
 
-class _MockGC(object):
-    def __init__(self):
-        self.callbacks = []
+        GCCollector(registry=self.registry)
+        self.registry.collect()
+        before = self.registry.get_sample_value('python_gc_objects_collected_total',
+                                                labels={"generation": "0"})
+        gc.collect(0)
+        self.registry.collect()
 
-    def start_gc(self, info):
-        for cb in self.callbacks:
-            cb('start', info)
+        after = self.registry.get_sample_value('python_gc_objects_collected_total',
+                                               labels={"generation": "0"})
+        self.assertEqual(0, after - before)
 
-    def stop_gc(self, info):
-        for cb in self.callbacks:
-            cb('stop', info)
+    def tearDown(self):
+        gc.enable()