Add HEAD_ISLOCKED to wrap GIL check

Feature or enhancement

Proposal:

This issue is extracted from #125561.
When interpretation_clear is changed, the PyThreadState_Clear function needs to be called when HEAD_LOCK is not held, but in fact we cannot be sure, but it existed before the change. In fact, PR solves the competition, but does not eliminate the risk of deadlock, so I give the following solution.

For PyThreadState_Clear, I want to add a HEAD_ISLOCKED macro to wrap PyMutex_IsLocked. This allows a check to be made when UNLOCK is called. Otherwise, if HEAD_LOCK is not called in the context, an is unlock error will occur when HEAD_UNLOCK is called. But in fact, we cannot be sure whether HEAD_LOCK is called in the context, and HEAD_IS_LOCK can be used to check it.

changed

diff --git a/Python/pystate.c b/Python/pystate.c
index 6b4e1d470f8..dc67e50bc27 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -796,6 +796,7 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
         // See https://github.com/python/cpython/issues/102126
         // Must be called without HEAD_LOCK held as it can deadlock
         // if any finalizer tries to acquire that lock.
+        HEAD_UNLOCK(&_PyRuntime);
         PyThreadState_Clear(p);
         p = p->next;
     }

The above code will show no locked when the context does not hold HEAD_LOCK, but this is not certain. So I want to add an if statement in front to unlock it when holding HEAD_LOCK. Ensure that there is no deadlock when calling PyThreadState_Clear.

if (HEAD_ISLOCKED(&_PyRuntime)) {
    HEAD_UNLOCK(&_PyRuntime);
}
PyThreadState_Clear(p);

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Eh, this seems like the wrong approach. I would be more comfortable with either making the mutex recursive, rather than implicitly releasing it for cases like this.

I wonder whether it's time to change this lock to recursive lock, we have had issues about deadlocks where re-entrant calls can deadlock, ex Py_DECREF.

Maybe we can wait for a solution to the problem.

Maybe we can wait for a solution to the problem.

The solution to that problem is to make it a recursive mutex :)

I don't think we want to use a recursive mutex for the linked list of thread states (or interpreter states). That pattern is still prone to lock ordering deadlocks with other threads because PyThreadState_Clear() can call arbitrary code via destructors.

(I also think that releasing the mutex implicitly is not the right strategy either)

Is it necessary to introduce the macro I mentioned for safely unlocking HEAD_LOCK? It can safely release the lock and eliminate errors caused by releasing when the context is not held.

I'll defer to Sam, as he knows much more about locking than I do, but I'm -1 on this, because it's generally a bad idea to implicitly release locks.

In fact, the current change solves the competition, but it does not solve the problem of probability deadlock.

Navigation Menu

Repository navigation

Add HEAD_ISLOCKED to wrap GIL check #125908

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Add HEAD_ISLOCKED to wrap GIL check #125908

Description

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Activity

ZeroIntensity commented on Oct 24, 2024

rruuaanng commented on Oct 24, 2024

ZeroIntensity commented on Oct 24, 2024

colesbury commented on Oct 25, 2024

rruuaanng commented on Oct 26, 2024

ZeroIntensity commented on Oct 26, 2024

rruuaanng commented on Oct 26, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions