🐛 [#423] fix reference cycle #434
Conversation
|
This is quite impressive! I don't yet understand how the reference cycle is created but I will take a closer look tomorrow. Thanks for your work! |
|
In the end, I found more proper way to prevent reference cycle, but it is a sort of tricky. |
|
Patching the class itself will cause issues if one instantiates multiple instances of thread and starts them. We don't want to wrap |
|
I fixed it by passing the |
|
Is there any progress about this pr? We also meet this problem in production. |
IIUC, this is still not thread safe. When you do this self.__class__.run = PatchedInstanceMethodDescriptor(
self.__class__.run, hub_
)it affects the entire Thread class, not just the single instance which is intended. |
|
How about something like this (untested): diff --git a/sentry_sdk/integrations/threading.py b/sentry_sdk/integrations/threading.py
index 3bd6032..5907b45 100644
--- a/sentry_sdk/integrations/threading.py
+++ b/sentry_sdk/integrations/threading.py
@@ -2,7 +2,7 @@ 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
@@ -35,26 +35,24 @@ class ThreadingIntegration(Integration):
else:
hub_ = Hub(hub)
- self.run = _wrap_run(hub_, self.run)
+ # Use the function object rather than the method
+ # descriptor to avoid a reference cycle (issue #434).
+ old_run = self.run.__func__
+ def sentry_run(*a, **kw):
+ hub = hub_ or Hub.current
+ with hub:
+ try:
+ self = current_thread()
+ return old_run(self, *a, **kw)
+ except Exception:
+ reraise(*_capture_exception())
+ self.run = sentry_run
return old_start(self, *a, **kw) # type: ignore
Thread.start = sentry_start # type: ignore
-def _wrap_run(parent_hub, old_run):
- def run(*a, **kw):
- hub = parent_hub or Hub.current
-
- with hub:
- try:
- return old_run(*a, **kw)
- except Exception:
- reraise(*_capture_exception())
-
- return run
-
-
def _capture_exception():
hub = Hub.current
exc_info = sys.exc_info() |
In my opinion, pathing |
|
I really like that last fix! |
|
Thanks @GuangTianLi ! |
Fixes #423
According to the Python Data Model, the code below will cause a reference cycle between
__self__and__dict__.The references graph like this::
Meantime Python Standard Library - concurrent.futures used thread to manage tasks queue, therefore when sentry patched start method of
Thread, it will cause reference cycle and prevent gc from collecting.So in
concurrent.futures.process, the_queue_management_threadin _threads_wakeups will be discarded until running a full gc collection, if not, exception will be raise when python exited, because the actual manage thread has already stopped.