🐛 [getsentry#423] fix reference cycle (getsentry#434)

GuangTianLi · untitaker · commit 5b3412ddbf8b · 2019-08-06T11:44:43.000+02:00
* 🐛 [getsentry#423] fix reference cycle * 🐛 [getsentry#423] fix reference cycle using descriptor * 🐛 [getsentry#423] patch multiple instance * 🐛 [getsentry#423] compat with python2 and pypy * test: Add tests * doc: Add executive summary * 🎨 [getsentry#423] format code with black * 🐛 [getsentry#423] fix reference cycle with an easier way
diff --git a/sentry_sdk/integrations/threading.py b/sentry_sdk/integrations/threading.py
@@ -1,15 +1,13 @@
 from __future__ import absolute_import
 
 import sys
-
-from threading import Thread
+from threading import Thread, current_thread
 
 from sentry_sdk import Hub
 from sentry_sdk._compat import reraise
-from sentry_sdk.utils import event_from_exception
-from sentry_sdk.integrations import Integration
-
 from sentry_sdk._types import MYPY
+from sentry_sdk.integrations import Integration
+from sentry_sdk.utils import event_from_exception
 
 if MYPY:
     from typing import Any
@@ -34,21 +32,26 @@ def sentry_start(self, *a, **kw):
                     hub_ = None
                 else:
                     hub_ = Hub(hub)
-
-                self.run = _wrap_run(hub_, self.run)
+                # Patching instance methods in `start()` creates a reference cycle if
+                # done in a naive way. See
+                # https://github.com/getsentry/sentry-python/pull/434
+                #
+                # In threading module, using current_thread API will access current thread instance
+                # without holding it to avoid a reference cycle in an easier way.
+                self.run = _wrap_run(hub_, self.run.__func__)
 
             return old_start(self, *a, **kw)  # type: ignore
 
         Thread.start = sentry_start  # type: ignore
 
 
-def _wrap_run(parent_hub, old_run):
+def _wrap_run(parent_hub, old_run_func):
     def run(*a, **kw):
         hub = parent_hub or Hub.current
-
         with hub:
             try:
-                return old_run(*a, **kw)
+                self = current_thread()
+                return old_run_func(self, *a, **kw)
             except Exception:
                 reraise(*_capture_exception())
 
diff --git a/tests/integrations/threading/test_threading.py b/tests/integrations/threading/test_threading.py
@@ -1,3 +1,5 @@
+import gc
+
 from threading import Thread
 
 import pytest
@@ -62,3 +64,45 @@ def stage2():
         assert event["tags"]["stage1"] is True
     else:
         assert "stage1" not in event.get("tags", {})
+
+
+def test_circular_references(sentry_init, request):
+    sentry_init(default_integrations=False, integrations=[ThreadingIntegration()])
+
+    gc.collect()
+    gc.disable()
+    request.addfinalizer(gc.enable)
+
+    class MyThread(Thread):
+        def run(self):
+            pass
+
+    t = MyThread()
+    t.start()
+    t.join()
+    del t
+
+    assert not gc.collect()
+
+
+def test_double_patching(sentry_init, capture_events):
+    sentry_init(default_integrations=False, integrations=[ThreadingIntegration()])
+    events = capture_events()
+
+    class MyThread(Thread):
+        def run(self):
+            1 / 0
+
+    ts = []
+    for _ in range(10):
+        t = MyThread()
+        t.start()
+        ts.append(t)
+
+    for t in ts:
+        t.join()
+
+    assert len(events) == 10
+    for event in events:
+        exception, = event["exception"]["values"]
+        assert exception["type"] == "ZeroDivisionError"
