From 5fdb00d3a5b6b7ef65f181c7e1c142f073138ac4 Mon Sep 17 00:00:00 2001 From: Hai Shi Date: Wed, 7 Apr 2021 01:27:39 +0800 Subject: [PATCH 1/2] Fix a reference leak if a thread is not joined. --- Lib/test/test_threading.py | 7 +++++++ Lib/threading.py | 10 ++++++++++ .../Library/2021-04-07-01-33-40.bpo-37788.F0tR05.rst | 1 + 3 files changed, 18 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2021-04-07-01-33-40.bpo-37788.F0tR05.rst diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 933935ba2ce2c8..fafb5318143f4e 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -874,6 +874,13 @@ def __call__(self): thread.join() self.assertTrue(target.ran) + def test_leak_without_join(self): + # bpo-37788: Test that a thread which is not joined explicitly + # does not leak. Test written for reference leak checks. + def noop(): pass + with threading_helper.wait_threads_exit(): + threading.Thread(target=noop).start() + # Thread.join() is not called class ThreadJoinOnShutdown(BaseTestCase): diff --git a/Lib/threading.py b/Lib/threading.py index ff2624a3e1e49e..c2becfbf3bbf55 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -839,6 +839,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: + if lock.acquire(False): + lock.release() + with _shutdown_locks_lock: + _shutdown_locks.discard(lock) + def _reset_internal_locks(self, is_alive): # private! Called by _after_fork() to reset our internal locks as # they may be in an invalid state leading to a deadlock or crash. diff --git a/Misc/NEWS.d/next/Library/2021-04-07-01-33-40.bpo-37788.F0tR05.rst b/Misc/NEWS.d/next/Library/2021-04-07-01-33-40.bpo-37788.F0tR05.rst new file mode 100644 index 00000000000000..d9b1e82b92238a --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-04-07-01-33-40.bpo-37788.F0tR05.rst @@ -0,0 +1 @@ +Fix a reference leak if a thread is not joined. From 213cd7727ac8badd6efc4737baee10c463c530ae Mon Sep 17 00:00:00 2001 From: Hai Shi Date: Fri, 9 Apr 2021 02:58:42 +0800 Subject: [PATCH 2/2] add _discard_shutdown_lock() in threading module --- Lib/test/test_threading.py | 14 ++++++++++++++ Lib/threading.py | 17 +++++++---------- Modules/_threadmodule.c | 25 ++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index fafb5318143f4e..a458c463eb6195 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -882,6 +882,20 @@ def noop(): pass threading.Thread(target=noop).start() # Thread.join() is not called + def test_leak_without_join_2(self): + # Same as above, but a delay gets introduced after the thread's + # Python code returned but before the thread state is deleted. + def random_sleep(): + seconds = random.random() * 0.010 + time.sleep(seconds) + + def f(): + random_sleep() + + with threading_helper.wait_threads_exit(): + threading.Thread(target=f).start() + # Thread.join() is not called + class ThreadJoinOnShutdown(BaseTestCase): diff --git a/Lib/threading.py b/Lib/threading.py index c2becfbf3bbf55..9fd5009421eb17 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -839,16 +839,6 @@ 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: - if lock.acquire(False): - lock.release() - with _shutdown_locks_lock: - _shutdown_locks.discard(lock) - def _reset_internal_locks(self, is_alive): # private! Called by _after_fork() to reset our internal locks as # they may be in an invalid state leading to a deadlock or crash. @@ -1417,6 +1407,13 @@ def _register_atexit(func, *arg, **kwargs): _main_thread = _MainThread() +def _discard_shutdown_lock(lock): + """ + Discard the Thread._tstate_lock lock when the non-daemon thread have done. + """ + with _shutdown_locks_lock: + _shutdown_locks.discard(lock) + def _shutdown(): """ Wait until the Python thread state of all non-daemon threads get deleted. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 0613dfd3070c5c..794f58d810f0fd 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -3,8 +3,9 @@ /* Interface to Sjoerd's portable C thread library */ #include "Python.h" -#include "pycore_pylifecycle.h" #include "pycore_interp.h" // _PyInterpreterState.num_threads +#include "pycore_pyerrors.h" // _PyErr_Occurred() +#include "pycore_pylifecycle.h" #include "pycore_pystate.h" // _PyThreadState_Init() #include // offsetof() #include "structmember.h" // PyMemberDef @@ -20,6 +21,7 @@ _Py_IDENTIFIER(__dict__); _Py_IDENTIFIER(stderr); _Py_IDENTIFIER(flush); +_Py_IDENTIFIER(threading); // Forward declarations @@ -1277,19 +1279,40 @@ In most applications `threading.enumerate()` should be used instead."); static void release_sentinel(void *wr_raw) { + _Py_IDENTIFIER(_discard_shutdown_lock); PyObject *wr = _PyObject_CAST(wr_raw); /* Tricky: this function is called when the current thread state is being deleted. Therefore, only simple C code can safely execute here. */ PyObject *obj = PyWeakref_GET_OBJECT(wr); lockobject *lock; + if (obj != Py_None) { lock = (lockobject *) obj; if (lock->locked) { PyThread_release_lock(lock->lock_lock); lock->locked = 0; } + PyThreadState *tstate = _PyThreadState_GET(); + PyObject *threading = _PyImport_GetModuleId(&PyId_threading); + if (threading == NULL) { + if (!_PyErr_Occurred(tstate)) { + _PyErr_SetString(tstate, PyExc_RuntimeError, + "lost threading module"); + } + goto exit; + } + PyObject *result = _PyObject_CallMethodIdOneArg( + threading, &PyId__discard_shutdown_lock, obj); + if (result == NULL) { + PyErr_WriteUnraisable(threading); + } else { + Py_DECREF(result); + } + Py_DECREF(threading); } + +exit: /* Deallocating a weakref with a NULL callback only calls PyObject_GC_Del(), which can't call any Python code. */ Py_DECREF(wr);