[Python-Dev] Change definition of Py_END_ALLOW_THREADS?

[Guido van Rossum]

> > > However, to do this auto thread state work properly, I will
> > > need to change the definition of Py_END_ALLOW_THREADS.
> > > Specifically, Py_END_ALLOW_THREADS needs to be able to
> > > handle the fact that the lock may or may not be held
> > > when it is called (whereas now, the lock must *not* be held)
> >
> > Can you explain how the current thread, which explicitly released the
> > lock in Py_BEGIN_ALLOW_THREADS, could end up owning the lock when
> > Py_END_ALLOW_THREADS is entered?
> 
> The short answer is that the current thread releases the lock (via
> Py_BEGIN_ALLOW_THREAD), then makes the call it needs to.  This call
> may synchronously call back into Python code, acquiring the lock.
> Currently, the rule is that the callback must also release the lock
> when done.  However, I am hoping to change things so that the
> callback code need *not* re-release the lock when on the same
> thread.

Heh?  The point of Py_BEGIN_ALLOW_THREAD is to allow *other* threads
to run, while the current thread engages in actions that could block
indefinitely.  The code between BEGIN and END can cause any number of
callbacks to run.  If one of those callbacks grabs the lock and
doesn't release it, no other thread will be able to run, even if a
later callback causes the current thread to block waiting for an
external event!

> The longer answer is to *why* I want to make that change:
> 
> Consider:
> 
> void SomeCallback()
> {
> 	PyAutoThreadState_Ensure();
>       ... do some Python API stuff.
>       SomeOtherFunc(); // my helper function.
>       ... more Python stuff.
>       PyAutoThreadState_Release();
> }
> 
> Now, consider SomeOtherFunc():
> 
> void SomeOtherFunc()
> {
> 	// This function also needs to use the Python API
> 	// but is called from many many places, where the
> 	// Python context is not always known.
> 	// So use the auto-thread-state
> 	PyAutoThreadState_Ensure();
> 	.. use the api
> 	PyAutoThreadState_Release();
> }
> 
> As you can see, we have *nested* calls to PyAutoThreadState_Release().
> The question is, in what state should PyAutoThreadState_Release()
> leave Python?

This suggests that the GIL needs to become a reentrant lock.

> To maintain the current semantics, the state must be left *exactly* as it
> was when we did the PyAutoThreadState_Ensure().  This would involve keeping
> state for each nested AutoThreadState call, which is a PITA, and I believe
> unnecessary. PyAutoThreadState_Release() can obviously not unconditionally
> release the lock, else it will not be held by SomeCallback() once the call
> to SomeOtherFunc() returns.
> 
> The semantics I hope to implement are that the Python GIL is only released
> when the *last* nested AutoThreadState call on a thread completes.  This
> should also help perf, as we can avoid a number of lock transitions.

Yes, releasing on the last call makes sense -- that's a reentrant lock.

> So, if we consider an extension module:
> PyObject *MyExtFunc(PyObject *args, PyObject *self)
> {
>    ...
>    Py_BEGIN_ALLOW_THREADS
>    // note the more common case is that "SomeCallBack" is
>    // invoked *indirectly* via the function being called - but
>    // for the sake of demo, let's assume a direct call.
>    SomeCallback();
>    Py_END_ALLOW_THREADS
> }
> 
> When SomeCallBack() completes, the PyAutoThreadState_Release() call realizes
> that this is *not* the bottom of the stack for this thread, so does not
> bother to release the lock.  Thus, when Py_END_ALLOW_THREADS is hit, we find
> that the current thread still has the lock.

The bottom of what stack?  If the GIL were a reentrant lock, I don't
see the problem.

> However, if "SomeCallBack()" had been called indirectly on another thread,
> then PyAutoThreadState_Release() *would* release the lock, as that call will
> be the final _Release() for that thread.

Sorry, I don't follow.

> ...
> 
> > If the issue is that there is (indeed very likely) code out there that
> > calls PyEval_RestoreThread() rather than using the macro, would it be
> > possible to change PyEval_RestoreThread() so that it acts the same as
> > PyEval_EnsureThread()?
> 
> Yes, this is probably best, but means the doc for PyEval_RestoreThread would
> change - in particular:
> 
> : If the lock has been created, the current thread must not have
> : acquired it, otherwise deadlock ensues.
> 
> However, as we are lossening this up, I see no real problem.

+1.

--Guido van Rossum (home page: http://www.python.org/~guido/)