[subinterpreters] Make the PyGILState API compatible with multiple interpreters

[pitrou]

BPO	10915
Nosy	@loewis, @mhammond, @ronaldoussoren, @amauryfa, @ncoghlan, @pitrou, @vstinner, @phsilva, @encukou, @ericsnowcurrently, @soltysh
Dependencies	bpo-10914: Python sub-interpreter test
Superseder	bpo-15751: [subinterpreters] Make the PyGILState API compatible with subinterpreters
Files	gilstateinterp.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-05-15.01:26:55.685>
created_at = <Date 2011-01-15.14:52:39.797>
labels = ['expert-subinterpreters', 'type-feature', '3.8', '3.9']
title = '[subinterpreters] Make the PyGILState API compatible with multiple interpreters'
updated_at = <Date 2020-05-15.01:26:55.684>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2020-05-15.01:26:55.684>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-05-15.01:26:55.685>
closer = 'vstinner'
components = ['Subinterpreters']
creation = <Date 2011-01-15.14:52:39.797>
creator = 'pitrou'
dependencies = ['10914']
files = ['20417']
hgrepos = []
issue_num = 10915
keywords = ['patch']
message_count = 22.0
messages = ['126333', '126335', '126348', '126349', '126351', '126354', '126355', '126357', '126363', '126364', '126366', '126387', '126400', '126402', '126406', '206041', '206047', '261778', '334266', '334451', '358263', '368903']
nosy_count = 13.0
nosy_names = ['loewis', 'mhammond', 'ronaldoussoren', 'amaury.forgeotdarc', 'ncoghlan', 'pitrou', 'vstinner', 'phsilva', 'grahamd', 'petr.viktorin', 'python-dev', 'eric.snow', 'maciej.szulik']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = 'resolved'
status = 'closed'
superseder = '15751'
type = 'enhancement'
url = 'https://bugs.python.org/issue10915'
versions = ['Python 3.8', 'Python 3.9']

[pitrou]

It should be relatively easy to devise a new PyGILState API with support for multiple interpreters. We just need two new functions (similar to the two existing ones) taking a PyInterpreterState* parameter; a TLS key can be added to the PyInterpreterState structure (instead of the current global TLS key).

It will be up to the caller to know which interpreter they want to hook into when calling these functions (which is application-dependent and is normally well-defined, e.g. when calling a Python callback, you should call it with the interpreter which was in use when registering the callback (i.e. PyThreadState_Get()->interp)).

[pitrou]

Here is a sketch, including conversion of ctypes to the new API.
Converting sqlite would require a bit more work.

[grahamd]

Can you please provide an example of what user would do and what changes existing extension modules would need to make?

When I looked at this exact problem some time back, I worked out that you probably only need a single new public API function. This would be something like PyInterpreterState_Swap().

By default stuff would work on the main interpreter, but if for a specific thread it wanted to operate in context of a different sub interpreter, would call PyInterpreterState_Swap() to indicate that. That would store in TLS outside of any existing data structures. Functions like existing PyGILState_Ensure()/PyGILState_Release() would then look up that TLS variable to know which interpreter they are working with.

Doing it this way meant that no C extension modules using PyGILState_??? functions would need to change at all, as what interpreter is being operated on dictated by who created the thread and initiated call in to Python interpreter.

You probably want validation checks to say that PyInterpreterState_Swap() can only be called when not GIL lock held.

It worries me that you are talking about new PyGILState_??? functions as that would suggest to me that extension modules would need to change to be aware of this stuff. That you are saying that sqlite needs changes is what makes me things the way you are going is a problem. It isn't practical to make SWIG change to use something other than PyGILState_Ensure()/PyGILState_Release(), it should be transparent and required no changes to existing C extensions.

[pitrou]

Can you please provide an example of what user would do and what
changes existing extension modules would need to make?

The patch contains such a change for ctypes. It's quite simple actually.

By default stuff would work on the main interpreter, but if for a
specific thread it wanted to operate in context of a different sub
interpreter, would call PyInterpreterState_Swap() to indicate that.
That would store in TLS outside of any existing data structures.
Functions like existing PyGILState_Ensure()/PyGILState_Release() would
then look up that TLS variable to know which interpreter they are
working with.

That sounds like an ugly hack to avoid passing the desired interpreter
state directly to PyGILState_Ensure()/PyGILState_Release().

Besides, it will only work if a thread always serves the same
sub-interpreter.

Doing it this way meant that no C extension modules using
PyGILState_??? functions would need to change at all, as what
interpreter is being operated on dictated by who created the thread
and initiated call in to Python interpreter.

Who would do that, if it's not the extensions in question?
"who created the thread" is often a third-party library (e.g. sqlite)
that has no notion of a Python interpreter. That's the whole point of
using the PyGILState_* API, really. So extensions *will* have to be
fixed.

That you are saying that sqlite needs changes is what makes me things
the way you are going is a problem. It isn't practical to make SWIG
change to use something other than
PyGILState_Ensure()/PyGILState_Release(), it should be transparent and
required no changes to existing C extensions.

What does SWIG use them for?

[ncoghlan]

A TLS based approach would presumably allow an embedding application like mod_wsgi to tinker with the state of threads created by naive modules that are unaware of the existence of subinterpreters.

That said, I don't see anything that prevents us from pursuing a TLS based override for the existing PyGILState functions later if the simpler, more explicit approach proves inadequate. As it stands, the new explicit calls allow something like mod_wsgi to define its *own* TLS location for the interpreter that is currently handling callbacks into Python, then use SWIG to generate PyGILState_*Ex calls in callback wrappers that reference that TLS interpreter state.

[pitrou]

A TLS based approach would presumably allow an embedding application
like mod_wsgi to tinker with the state of threads created by naive
modules that are unaware of the existence of subinterpreters.

The question is how mod_wsgi could know about the existence of these
threads, let alone decide which subinterpreter an arbitrary OS thread
should belong to; only the extension module can safely tell.
And it becomes totally hopeless if those threads are actually *shared*
between subinterpreters, as might be the case with a 3rd-party library
managing its own helper threads (I don't know if that's the case with
sqlite).

IMO we should really promote clean APIs which allow solving the whole
problem, rather than devise an internal hack to try to "improve" things
slightly.

[grahamd]

The bulk of use cases is going to be simple callbacks via the same thread that called out of Python in the first place. Thus ultimately all it is doing is:

Py_BEGIN_ALLOW_THREADS

Call into some foreign C library.
C library wants to do a callback into Python.

PyGILState_STATE gstate;
gstate = PyGILState_Ensure();

/* Perform Python actions here. */
result = CallSomeFunction();
/* evaluate result or handle exception */

/* Release the thread. No Python API allowed beyond this point. */
PyGILState_Release(gstate);

More stuff in C library.
Return back into the C extension wrapper.

Py_END_ALLOW_THREADS

This is what SWIG effectively does in its generated wrappers for callbacks.

Using a TLS solution, all these modules that simply do this will now start working where as they currently usually deadlock or have other problems.

In your solution, all these modules would need to be modified to some how transfer information about the current interpreter into the callback which is called by the foreign C library and use new PyGILState_??? functions rather than the old.

I do accept that more complicated extension modules which create their own foreign threads and perform the call back into interpreter from that thread, or systems like mod_wsgi which have a persistent thread pool from which calls originate, will have to be modified, but this is the lessor use case from what I have seen.

Overall, it is an easy win if TLS is used because a lot of code wouldn't need to change. Some will, but expect that a lot of the common stuff like lxml for example wouldn't.

[grahamd]

As to the comment:

"""IMO we should really promote clean APIs which allow solving the whole
problem, rather than devise an internal hack to try to "improve" things
slightly."""

The reality is that if you force a change on every single extension module doing callbacks into the interpreter without having the GIL first, you will never see people update their code as they will likely not care about this special use case. And so the whole point of adding the additional APIs will be wasted effort and have achieved nothing.

The TLS solution means many modules will work without the authors having to do anything.

You therefore have to balance between what you perceive as a cleaner API and what is actually going to see a benefit without having to wait a half dozen years before people realise they should change their ways.

BTW, TLS is currently used for current thread state for simplified GIL API, why isn't that use of TLS a hack where as doing the same for interpreter is?

[pitrou]

The bulk of use cases is going to be simple callbacks via the same
thread that called out of Python in the first place. [...]
This is what SWIG effectively does in its generated wrappers for
callbacks.

Thanks for clarifying. I agree the TLS scheme would help in these cases.
There is still the question of what/who updates the TLS mapping; you are
proposing a new API call; an alternative is to do the mapping in e.g.
PyEval_SaveThread().

The reality is that if you force a change on every single extension
module doing callbacks into the interpreter without having the GIL
first, you will never see people update their code as they will likely
not care about this special use case. And so the whole point of adding
the additional APIs will be wasted effort and have achieved nothing.

I'm not sure I care. If people don't want to use the new APIs on the
basis that they are slightly more complex, then it's their problem. The
Python C API tries not to be too cumbersome, but it cannot pretend to be
as transparent as a high-level API in a dynamic language.

[ncoghlan]

There's no point improving the multiple interpreter support if it doesn't help applications that are currently broken because of that lack of support.

I believe the patch as currently proposed actually makes things *worse*. With "autoTLSkey" as a static variable, all subinterpreters will use the same key to point into thread local storage (which is defined process-wide). This means they won't tread on each other's toes: the interpreter that creates a thread owns that thread. So Graham's simple use case *should already work*, as the creation of the thread by the subinterpreter will populate autoTLSkey with a valid thread state, which will then be used by calls back in to the GILState API.

Looking at 3.2, there appear to be two ways for an application to get itself into trouble:

1. Hand off an OS level thread from the creating interpreter to a different subinterpreter. As far as I can tell, calling GILState_Ensure in such a thread will still acquire the GIL of the creating interpreter (or something equally nonsensical). I may be misreading that though - this isn't exactly the easiest part of the code base to follow :)

2. Native (non-Python) threads will always have their temporary thread state created in the main interpreter unless the application takes steps to precreate a thread state using a different interpreter.

So, a new PyGILState_EnsureEx API may be beneficial regardless in order to help with part 2 of the problem, but I think it should be used solely as a way to override "autoInterpreterState". "autoTLSkey" should be left alone so that a given OS level thread can only be owned by one interpreter at a time.

Further, there is no need for any function with access to a valid thread state (i.e. _PyGILState_NoteThreadState, as well as PyGILState_Release if autoTLSkey remains a static variable) to take an interpreter argument. These functions can identify the relevant interpreter from the "interp" field of the thread state.

TL;DR version:

• I agree the compatibility between multiple interpreters and the GILState API can be improved
• I agree a PyGILState_EnsureEx that takes an interpreter argument should be part of that solution.
• I *disagree* with making autoTLSkey interpreter specific, as it seems to me that will make the situation worse rather than better.

[ncoghlan]

Added Mark Hammond to the nosy list, as the original author and implementor of PEP-311 (which added the GILState APIs).

Mark, since your PEP deliberately punted on multiple interpreter support, feel free to take yourself off the list again. If you spot any glaring errors in my post above it would be nice to know, though :)

[grahamd]

Nick, I think you are making the wrong assumption that an external threads will only ever call into the same interpreter. This is not the case. In mod_wsgi and mod_python there is a pool of external threads that for distinct HTTP requests, delegated to a specific thread, can make calls into different interpreters. This is all fine so long as you ensure that for each thread, it uses a distinct thread state for that thread for each interpreter. In other words, you cant use the same thread state instance across multiple interpreters as it is bound to a specific interpreter.

This is because autoInterpreterState is always going to be set to the main interpreter. This means that when the thread is calling into a new sub interpreter it will either inherit via current GIL state API an existing thread state bound to the main interpreter, or if one is created, will still get bound to the main interpreter. As soon as you start using a thread state bound to one interpreter against another, problems start occurring.

After thinking about this all some more I believe now what is needed is a mix of the TLS idea for current interpreter state that I am suggesting and in part the extended GIL state functions that Antoine describes.

So, the TLS records what interpreter a thread is currently running against so that GIL state APIs work for existing unmodified extension modules. At the same time though, you still need a way of switching what interpreter a thread is running against. For the latter, various of the thread state related functions that exist already could do this automatically. In some cases you will still need the extended function for acquisition that Antoine suggested.

Consider a few scenarios of usage.

First off, when an external thread calls PyInterpreter_New(), it creates a new thread state object against that new sub interpreter automatically and returns it. With this new systems, it would also automatically update the TLS for the current thread to be that new interpreter also. That way when it calls into Python which then calls back out to code which releases the GIL and then calls back in through PyGILState_Ensure(), with no arguments, it will work. This obviously implies though that PyGILState_Ensure() makes use of the TLS for the interpreter being used and isn't hard wired to the main interpreter like it is now.

Second, consider some of the other API functions such as PyThreadState_Swap(). When passing it a non NULL pointer, you are giving it a thread state object which is already bound to an interpreter. It thus can also update the TLS for the interpreter automatically. If you pass it a NULL then it clears the TLS with all functions later that rely on that TLS asserting that it is not NULL when used. Another similar case where TLS can be auto updated is functions which clear/delete an interpreter state and leave GIL unlocked at the end. These also would clear the TLS.

So, it is possible that that no new API functions may be needed to manage the TLS for what interpreter is associated with the current thread, as I thought, as existing API functions can do that management themselves transparently.

The third and final scenario, and the one where the extended GIL state functions for Ensure is still required, is where code doesn't have the GIL as yet and wants to make a call into sub interpreter rather than the main interpreter, where it already has a pointer to the sub interpreter and nothing more. In this case the new PyGILState_EnsureEx() function is used, with the sub interpreter being passed as argument.

The beauty of existing API functions of PyThreadState_Swap() etc managing the TLS for the interpreter is that the only code that needs to change is the embedded systems which are creating and using multiple interpreters in the first place. In other words, mod_wsgi would need to change, with it simply replacing all the equivalent stuff it already has for doing what PyGILState_??? functions do now but against sub interpreters. If I am right, all extension modules that don't really care about whether sub interpreters are being used should work without modification.

Oh, and I also think you probably don't need PyGILState_ReleaseEx() if all made TLS aware, just the single PyGILState_EnsureEx() is needed.

[ncoghlan]

Graham - the cases you describe are the things I was saying don't currently work in my post and wouldn't be helped by Antoine's patch. Your thoughts on how we could possibly make it work actually parallel mine (although there may be fun and games with making sure the respective GILs are acquired and released in an appropriate order when switching interpreters).

However, if a given OS thread is created by a subinterpreter via thread_PyThread_start_new_thread, then the thread bootstrapping process will copy the thread state from that subinterpreter rather than the main interpreter.

Accordingly, the only thing that I believe should currently work with subinterpreters is the naive use case you described earlier (i.e. a call out to an extension module from a Python created thread that later calls back in using the PyGILState API in the exact same thread should work even in the presence of multiple interpreters).

This is the use case that I believe Antoine's patch as it currently stands actively *breaks* by making the autoTLSkey interpreter dependent.

Regardless, I'm marking 10914 as a dependency of this one, as I don't think we should change anything in this area until we have some unit tests to properly define what does and doesn't work. If we're going to promote subinterpreters to a robust, fully supported feature we may as well do it right.