bpo-37788: Fix a reference leak if a thread is not joined. #25226
Conversation
|
Those test cases have been passed in my local: |
|
It seems like the failed test case( |
Lib/threading.py
Outdated
| return | ||
| lock = self._tstate_lock | ||
| if lock is not None and not self.daemon: | ||
| if lock.acquire(False): |
There was a problem hiding this comment.
If the lock is not acquired, then the lock still remains in the global list and leaks.
There was a problem hiding this comment.
You are right. Running threads will still in interpreter.
import sys
import random
import threading
import time
def f():
seconds = random.random() * 0.01
time.sleep(seconds)
for _ in range(10):
threading.Thread(target=f).start()
print(sys.gettotalrefcount())
There was a problem hiding this comment.
Wait. this test case can not measure the refleaks. It will raise the refleaks too when I apply #15228.
|
Hm, there have an error in my local: |
|
Using And |
|
Hi, Antoine, Serhiy. Can you take a look when you have free time? I try to fix the refleak of threading after victor reverted his PR: #15228 @pitrou @serhiy-storchaka |
|
I don't like the idea of adding C code for this. You could try to make |
Hm. When I use The error info: The reason is that the interpreter can't use the weakref of |
|
Ah, right. The problem, though, is this comment at the top of
Your code is adding the execution of arbitrary Python code, which violates the requirement expressed in the comment. It seems to work, but it doesn't seem like a good idea to bet that it will always work. |
|
This patch is not the prettiest solution, but it seems to work: diff --git a/Lib/threading.py b/Lib/threading.py
index 4dcf84715c..e44e13666a 100644
--- a/Lib/threading.py
+++ b/Lib/threading.py
@@ -780,12 +780,27 @@ def _newname(name_template):
_active = {} # maps thread id to Thread object
_limbo = {}
_dangling = WeakSet()
+
# Set of Thread._tstate_lock locks of non-daemon threads used by _shutdown()
# to wait until all Python thread states get deleted:
# see Thread._set_tstate_lock().
_shutdown_locks_lock = _allocate_lock()
_shutdown_locks = set()
+def _maintain_shutdown_locks():
+ """
+ Drop any shutdown locks that don't correspond to running threads anymore.
+
+ Calling this from time to time avoids an ever-growing _shutdown_locks
+ set when Thread objects are not joined explicitly. See [bpo-37788](https://bugs.python.org/issue37788).
+
+ This must be called with _shutdown_locks_lock acquired.
+ """
+ # If a lock was released, the corresponding thread has exited
+ to_remove = [lock for lock in _shutdown_locks if not lock.locked()]
+ _shutdown_locks.difference_update(to_remove)
+
+
# Main class for threads
class Thread:
@@ -968,6 +983,7 @@ def _set_tstate_lock(self):
if not self.daemon:
with _shutdown_locks_lock:
+ _maintain_shutdown_locks()
_shutdown_locks.add(self._tstate_lock)
def _bootstrap_inner(self):
@@ -1023,7 +1039,8 @@ def _stop(self):
self._tstate_lock = None
if not self.daemon:
with _shutdown_locks_lock:
- _shutdown_locks.discard(lock)
+ # Remove our lock and other released locks from _shutdown_locks
+ _maintain_shutdown_locks()
def _delete(self):
"Remove current thread from the dict of currently running threads." |
|
Also cc @tim-one for advice. |
Nice patch. I have tested in my local and it can work :) |
|
How about maintain diff --git a/Lib/threading.py b/Lib/threading.py
index 4dcf84715c4..dd5d07ca1bd 100644
--- a/Lib/threading.py
+++ b/Lib/threading.py
@@ -857,6 +857,16 @@ class is implemented.
# For debugging and _after_fork()
_dangling.add(self)
+ def __del__(self):
+ if not self._initialized:
+ return
+ lock = self._tstate_lock
+ if lock is not None and not self.daemon:
+ with _shutdown_locks_lock:
+ to_remove = [lock for lock in _shutdown_locks
+ if not lock.locked()]
+ _shutdown_locks.difference_update(to_remove) |
|
|
|
@pitrou Hi, Antoine. Would you mind to create a new PR for your patch? |
|
Antoine's PR is more better than this PR, so I closed this PR ;) |
|
Ok, I've created PR #26103 for it. |
Add threading.Thread._discard_shutdown_lock() method to ensure that the thread state
lock is removed from the _shutdown_locks list when a thread
completes.
Co-authored-by: Victor Stinner vstinner@python.org
https://bugs.python.org/issue37788