gh-87135: threading.Lock: Raise rather than hang o 8000 n Python finalization #135991
Conversation
…lization After Python finalization gets to the point where no other thread can attach thread state, attempting to acquire a Python lock must hang. Raise PythonFinalizationError instead of hanging.
| @@ -95,6 +95,18 @@ _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout, _PyLockFlags flags) | |||
| if (timeout == 0) { | |||
| return PY_LOCK_FAILURE; | |||
| } | |||
| if ((flags & _PY_LOCK_PYTHONLOCK) && Py_IsFinalizing()) { | |||
There was a problem hiding this comment.
The first part of the condition here is checking if any of the bits of _PY_LOCK_PYTHONLOCK are set. In other words, it will pass even if only _PY_LOCK_DETACH is set from PyMutex_Lock().
It could be:
if ((flags & _PY_LOCK_PYTHONLOCK) == _PY_LOCK_PYTHONLOCK && Py_IsFinalizing()) {
But given that this is an easy mistake to make and hard to spot, I'd rather just have a separate bit for fail-on-finalization and check that individual bit here.
If we want a constant to combine those three bits, let's keep that constant it in _threadmodule.c.
There was a problem hiding this comment.
Won't this be prone to TOCTOU issues? Py_IsFinalizing could pass, but once the thread state detaches in the parking lot, the interpreter is free to start finalizing. The parking lot will then hang the thread upon reattachment in _PySemaphore_Wait, right?
There was a problem hiding this comment.
No - this check is only relevant for the main thread (the one that performs Py_Finalize()) so there's no TOCTOU. There are two "hanging" threads behaviors that you may be confusing:
- Non-main threads, typically daemon threads (and occasionally other non-daemon threads), hang when they try to attach during Python finalization. This PR doesn't affect that behavior.
- The main thread, when performing finalization, may acquire
threading.Lock/RLockcalled via destructors / GC. If those locks happen to be held by other (now hung) threads, then the shutdown would deadlock. This instead raises a Python exception.
There was a problem hiding this comment.
Hm, yeah, I was confusing it with that. But, you've mentioned before that acquiring Python locks in finalizers isn't safe regardless of finalization, because the GC might run in any code where the lock is held and deadlock while reacquiring it. Looking at the test case, I think that still applies. What's the use-case that this PR is fixing?
There was a problem hiding this comment.
Thanks for the catch! I was too elated to whittle this down to a (hopefully) nice small tweak... and forgot how bits work :/
Won't this be prone to TOCTOU issues?
What's the use-case that this PR is fixing?
Even if there was a TOCTOU, I think this PR would be useful.
In cases where an app sometimes hangs on shutdown, making it usually raise an exception instead means this case is easier to debug.
Acquiring Python locks in finalizers isn't safe, but a PythonFinalizationError is more likely to make people realize that.
There was a problem hiding this comment.
Ok, thanks for clearing it up.
There is a suspicious error when running test.test_multiprocessing_spawn.test_processes: |
After Python finalization gets to the point where no other thread can attach thread state, attempting to acquire a Python lock will hang.
Raise PythonFinalizationError instead of hanging.
@colesbury, does this look maintainable (and correct) to you?