classification
Title: Support subinterpreters in the GIL state API
Type: enhancement Stage: needs patch
Components: Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, eric.snow, grahamd, haypo, jcea, mhammond, ncoghlan, pitrou, python-dev
Priority: normal Keywords:

Created on 2012-08-21 05:18 by ncoghlan, last changed 2016-03-14 21:50 by python-dev.

Messages (43)
msg168742 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 05:18
Currently, modules that use the PyGILState* APIs cannot be used with mod_wsgi, as mod_wsgi uses the subinterpreter support.

Graham Dumpleton and I spent some time discussing this at PyCon AU 2012, and we believe that the incompatibility can be resolved with a single API in the core: a function that mod_wsgi can call to set the interpreter used by the GIL state APIs to implicitly create new thread states.

This is still only a seed of an idea (and it's entirely possible we've missed something), but it's a place to start in resolving this longstanding incompatibility.
msg168759 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-21 11:56
Just to clarify. One can still tell WSGI applications under mod_wsgi to run in the main interpreter and in that case modules using PyGILState* do work. By default though, sub interpreters are used as noted.

The mechanism for forcing use of main interpreter is the directive:

WSGIApplicationGroup %{GLOBAL}

Some details about this issue can be found in:

http://code.google.com/p/modwsgi/wiki/ApplicationIssues#Python_Simplified_GIL_State_API
msg168764 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 12:56
> a function that mod_wsgi can call to set the interpreter used by the
> GIL state APIs to implicitly create new thread states.

How would it work?
msg168766 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 13:21
It would twiddle the autoInterpreterState and autoTLSkey entries in the pystate.c global variables to point to a different subinterpreter.

As I understand the situation, mod_wsgi doesn't need arbitrary externally created threads to be able to call back into arbitrary subinterpreters, it just needs to be able to direct externally created threads in a process to a subinterpreter other than the main one.

Graham, looking at the current impl - have you experimented with just calling _PyGILState_Init() with the interpreter state and current thread state for the desired subinterpreter to see what happens?

I think the new method could just be a cleaner combination of _PyGILState_ReInit and _PyGILState_Init. If I'm right, then calling _PyGILState_Init should convert the current crashes and deadlocks into a relatively less harmful memory leak (since the old entry in the TLS won't get deleted properly).
msg168768 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 13:25
Graham, even better would be if you could try the following combination:

_PyGILState_Fini();
_PyGILState_Init(si, st);

(where si and st are the interpreter state and thread state for the target subinterpreter)

If a new PyGILState_SwitchInterpreter API is going to be able to solve this in 3.4, then I believe those private APIs should be enough to make it possible in *current* versions.

If those private APIs *aren't* enough, then I'm missing something and this isn't going to be as easy as I thought.
msg168769 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 13:28
> As I understand the situation, mod_wsgi doesn't need arbitrary
> externally created threads to be able to call back into arbitrary
> subinterpreters, it just needs to be able to direct externally created
> threads in a process to a subinterpreter other than the main one.

But how does it know that the right externally created thread will point
to the right subinterpreter? The OS is free to schedule threads as it
desires.
msg168771 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 14:06
Just as they do today, all externally created threads will still go to *one* interpreter when they hit PyGILState_Ensure(). It's just that interpreter won't be the main one.

Since the whole point of the PyGILState API is to support threads that don't have a previously created thread state, there's no getting around the requirement to have a single blessed interpreter that handles all externally created threads in a given process.

It will be up to mod_wsgi (and any other embedding application that uses the new function) to make sure it calls this at a time when there aren't any existing calls to PyGILState that would be disrupted. (Assuming we can't figure out a locking scheme that *ensures* no such threads are running when the switch occurs)
msg168772 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 14:10
> Just as they do today, all externally created threads will still go to
> *one* interpreter when they hit PyGILState_Ensure(). It's just that
> interpreter won't be the main one.

Uh but how does it solve the issue? (unless you create a mod_wsgi app
with only a single worker thread)
msg168776 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 14:33
My understanding of the mod_wsgi architecture is that it uses subinterpreters to maintain a persistent process, while still providing a relatively pristine interpreter state to handle each new request. This means even when you're using multiple processes with a single request handling thread per process, you're running a subinterpreter unless you explicitly configure mod_wsgi to always run in the main interpreter (which, I believe, will result in additional state persistence across requests).

The proposed API change can only fix scenarios where *at a given point in time*, *all* PyGILState_Ensure calls should be directed to a particular subinterpreter. The target subinterpreter may change *later*, but there still cannot be two desired targets within the same process at the same time.

However, at the moment, PyGILState doesn't even allow that - all externally created threads are handled in the *main* interpreter even if that isn't what the embedding application wants.
msg168779 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 14:44
Sorry, I mischaracterised the way mod_wsgi works slightly. However, my understanding is still that the scope of this particular fix is merely to allow all external threads to be redirected to a different subinterpreter at various times over the life of a process. It does not need to allow different external threads to be redirected to different subinterpreters.

(Note: I am assuming that any hooks Apache/mod_wsgi has into external thread creation could already just create an appropriate thread state if that was the desired behaviour. It may be I'm incorrect on this, and what Graham really wants is the ability to change the target interpreter state just for the current thread. However, if that's what he wants, then there's additional background info I need on mod_wsgi and its ability to influence thread creation within a process, because I didn't get the impression on the weekend that that is what he was after)
msg168780 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 14:47
> My understanding of the mod_wsgi architecture is that it uses
> subinterpreters to maintain a persistent process, while still
> providing a relatively pristine interpreter state to handle each new
> request.

I don't think that's true. On hg.python.org, the hglookup application
keeps a cached mapping of changeset ids to repo URLs, which wouldn't be
possible if its interpreter was re-bootstrapped for each new request.
msg168787 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 15:23
s/slightly/completely/ (I believe my description of the core problem was right, but my description of why that problem exists was wrong - it actually has to do with the way mod_wsgi handles virtual hosts and endpoints)

If we expose an official way to switch the destination of the PyGILState APIs, then what it means is that mod_wsgi can, over the lifecycle of a single process in the pool, *switch* the virtual host and WSGI endpoint that process is handling by changing the active interpreter. There are still some extension modules where that won't work (because they create persistent threads that periodically call back into Python, so they may still end up calling back in to the wrong interpreter), but it will allow those that just do callbacks from external worker threads (or other operations that are similarly bound by the lifecycle of a request) to start working properly.
msg168789 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 15:51
> If we expose an official way to switch the destination of the
> PyGILState APIs, then what it means is that mod_wsgi can, over the
> lifecycle of a single process in the pool, *switch* the virtual host
> and WSGI endpoint that process is handling by changing the active
> interpreter.

I don't understand what that means. It's the OS which switches threads,
including interpreter threads.

(by the way, the real fix to the GILState vs. subinterpreters issue
would be to create new API functions which take an interpreter as
argument, so that you have a distinct TLS key per interpreter)
msg168790 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-21 16:08
Umm, no. The whole point of the GILState API is that you can call it from a thread which knows *nothing* about Python.

It will then use the *process global* state in the pystate.c statics to initialise that thread with a Python thread state. Currently, that thread state will always be for the main Python interpreter for the process.

All Graham wants is an officially supported way to change which interpreter the pystate.c globals reference *without* shutting down and reinitialising Python completely.

There are going to be limitations on how effective this will be - it still won't support *all* extension modules that use the PyGILState APIs. It will, however, support many more of them than the status quo (which is zero, unless you force your WSGI app to use the main interpreter, which has its own problems).

And you absolutely can control when the OS switches threads - controlling that is what synchronisation primitives are *for*.
msg168792 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 16:27
> Umm, no. The whole point of the GILState API is that you can call it
> from a thread which knows *nothing* about Python.

No to what? Any sane callback API allows to pass it some user data, that
user data can just as well include the pointer to the desired
interpreter. Really, that's the right way to do it. Since a new API is
required, we can indeed add tweaks to the current API until the
migration is done, but I'm quite sure any proper solution to the problem
requires explicitly passing the interpreter.

> There are going to be limitations on how effective this will be - it
> still won't support *all* extension modules that use the PyGILState
> APIs. It will, however, support many more of them than the status quo

I still don't understand how that allows to "support some extension
modules" (which ones?).
A typical mod_wsgi setup uses several threads per daemon process, and
each thread (AFAIU) uses a separate subinterpreter for its WSGI
application instance (Graham, is that false?). Therefore, an
externally-launched thread can relate to *any* of these subintepreters,
which are themselves scheduled by the OS.

> And you absolutely can control when the OS switches threads -
> controlling that is what synchronisation primitives are *for*.

I don't think mod_wsgi has access to enough hooks or information to do
that satisfyingly (like the OS would do). Besides, I don't understand
how mod_wsgi could have control over the scheduling of
externally-launched threads. Therefore, an externally-launched thread
could be scheduled at any time, and not only after the "right"
interpreter thread was interrupted.
msg168812 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-21 22:14
In both embedded mode and daemon mode of mod_wsgi, albeit how thread pool is managed is different, there is a fixed number of threads with those being dedicated to handling web requests.

On a request arriving next available thread from the thread pool handles accepting of request at C code level, that thread may then map to any WSGI application and so any sub interpreter, or even the main interpreter.

Thus there is no one to one mapping between thread and (sub)interpreter.

The way the mod_wsgi code works now is that when it knows it will be calling into the main interpreter, it uses PyGILState_Ensure(). If not, it will use a thread state for that thread specific to the sub interpreter it is calling in to. At the end of the request, the thread state is remembered and not thrown away so that thread locals still work for that thread across requests for that sub interpreter.

Thus, there can be more than one thread state per thread, but this is fine so long as it is only used against the sub interpreter it was created for.

This is actually an enforced requirement of Python, because if you create more than one thread state for a thread for the same sub interpreter, or even an additional one for the main interpreter when there is also the auto TLS, then Python will die if you compile and run it is in debug mode.

Now, since mod_wsgi always knows which interpreter it is calling into, the intent was that there was this single API call so that mod_wsgi could say that at this time, this thread is going to be calling into that interpreter. It could then just call PyGILState_Ensure().

Any third party module then which uses the simplistic calling sequence of calling PyGILState_Release() on exiting Python code and thence within the same thread calling PyGILState_Ensure() when coming back into Python with a callback will work, as mod_wsgi has specified the interpreter context for that thread at that time.

As pointed out, if a third party module was creating its own background threads at C level and calling PyGILState_Ensure() when calling back into Python code, this could pose a problem. This could also be an issue for Python created background threads.

In the case of the latter, if a Python thread is created in a specific sub interpreter, it should automatically designate for that thread that that is its interpreter context, so if it calls out and does the Release/Ensure dance, that it goes back into the same sub interpreter.

The C initiated thread is a bit more complicated though and may not be solvable, but a lot of the main third party modules which don't work in sub interpreters, such as lxml, don't use background threads, so the simplistic approach means that will work at least.

So, in summary, saw a single API call which allowed designation of which interpreter a thread is operating against, overriding the implicit default of the main interpreter. PyGILState API will need to manage a set of interpreter states for each interpreter, with potential for more than one thread state for a thread due to a thread being able to call into multiple interpreters at different times.
msg168815 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 22:31
Le mardi 21 août 2012 à 22:14 +0000, Graham Dumpleton a écrit :
> Any third party module then which uses the simplistic calling sequence
> of calling PyGILState_Release() on exiting Python code and thence
> within the same thread calling PyGILState_Ensure() when coming back
> into Python with a callback will work, as mod_wsgi has specified the
> interpreter context for that thread at that time.

Why would a module do that, instead of using
Py_{BEGIN,END}_ALLOW_THREADS?
msg168816 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-21 22:36
Those macros only work for general GIL releasing and pop straight away, not for the case where released, calls into some non Python C library, which then calls back into Python.

My recollection is, and so unless they have changed it, SWIG generated calls use the GILState calls. See:

https://issues.apache.org/jira/browse/MODPYTHON-217
msg168818 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-21 22:56
> Those macros only work for general GIL releasing and pop straight
> away, not for the case where released, calls into some non Python C
> library, which then calls back into Python.

I see, so you are right that this new API could be useful. However, we
should also add a new GIL state API that fixes the issue for good (by
passing an interpreter), otherwise we will still have such discussions
in five years and it will be very silly.

> My recollection is, and so unless they have changed it, SWIG generated
> calls use the GILState calls. See:

Well, if SWIG isn't fixed, people should stop using an unmaintained and
buggy tool.
msg168832 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-22 00:46
If you have a Ex version of Ensure, then if the interpreter argument is NULL, then it should assume main interpreter. That way the normal version of Ensure can just call PyGILState_EnsureEx(NULL).
msg169008 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-24 12:35
I'm not sure it makes sense to call this new API "PyGILState_EnsureEx". My concern is that the behaviour is quite different in the presence of an existing thread state:

Ensure:
- if a thread state exists, use that interpreter
- otherwise, use the default interpreter configured in the pystate.c globals

New API:
- if a thread state exists, and the interpreter doesn't match the requested one, fail with an error
- otherwise, use the requested interpreter

I guess it makes sense if we treat the NULL pointer as the degenerate case meaning "use the interpreter of this thread, or the default interpreter if no interpreter has been declared for this thread". PyGILState_Ensure would then simply call PyGILState_EnsureEx(NULL) internally.

So, my question for Graham would be, given this ability, would mod_wsgi still need the ability to change the default interpreter? Or would it be enough for you to be able to register the threads *you* create with a specific interpreter?
msg169010 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-24 12:45
> New API:
> - if a thread state exists, and the interpreter doesn't match the
> requested one, fail with an error
> - otherwise, use the requested interpreter

That's not what I'm proposing. What I'm proposing is that the new API
uses a per-interpreter TLS key (so you can have several thread states
per OS thread).

So basically:

Ensure:
- look up global TLS key, which returns the thread state
- if no thread state (TLS lookup failed), create a new one for the main
interpreter and register it on the global TLS key

New API:
- look up the interpreter's TLS key, which returns the thread state
- if no thread state (TLS lookup failed), create a new one for the
interpreter and register it on the interpreter's TLS key

Graham is merely suggesting for simplification that "global TLS key" ==
"main interpreter's TLS key", so Ensure(...) ==
EnsureEx(main_interpreter, ...).
msg169013 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-24 12:55
And the current "autoTLSkey" could move into the interpreter state object? I like it - that's a lot more flexible than the approach I was thinking of.
msg169014 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-24 13:00
It is past my bed time and not thinking straight, but I believe Antoine is aligned with what I had in mind, as need multiple thread states per OS thread where each is associated with separate interpreter.

My main reason for allowing NULL to EnsureEX rather than requiring main_interpreter to be explicitly passed, is that way way back in time, my recollection is that getting access to the main interpreter pointer was a pain as you had to iterate over the list of interpreters and assume it was the last one due to it being created first. I don't remember there being a special global variable or function for getting a pointer to the main interpreter. This may well have changed since and there is an easier way do let me know. So saw it as a convenience.
msg169278 - (view) Author: Mark Hammond (mhammond) * (Python committer) Date: 2012-08-28 13:21
The GIL state api was mainly interested in the case of a thread which has (possibly) never been seen before calling into Python.  IIUC, the proposal here is so that a thread that *has* been seen before can be associated with a thread-state specified by the embedding application (and the degenerate case would be to assume the thread hasn't been seen, and as such should get the default interpreter)

If that isn't too wide of the mark, I agree it sounds workable and worthwhile.
msg169280 - (view) Author: Mark Hammond (mhammond) * (Python committer) Date: 2012-08-28 13:36
To clarify, I wrote:

> can be associated with a thread-state specified by the 
> embedding application 

Where I meant to say:

Can be associated with an interpreter state and corresponding thread-state ...

Or something like that - it's been a while since I've looked at that code.
msg169284 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-28 14:12
Thinking about it, I believe there still needs to be a concept of an "active thread state" TLS key in order to deal with Graham's original problem. Specifically, if PyGILState_EnsureEx is used to associate the thread with a particular interpreter, then subsequent calls to PyGILState_Ensure from *that thread* should get the explicitly associated interpreter, rather than the main interpreter.

Then, the lookup sequence for "interpreter=NULL" would be:

1. Check the active TLS key, if set, use that thread state
2. If not set, look up the main interpreter's TLS key. If set, also set that on the active TLS key and use that thread state.
3. If still not set, create a new thread state on the main interpreter, set it for both the active TLS and the main interpreter's TLS key and use that thread state

A similar approach almost works when requesting a specific interpreter, but where that goes wrong is when the active TLS key is *already set*. You can't just overwrite it, because that will cause problems for subsequent PyGIL_Release calls. You could just make it an error, but I think Graham's original idea makes it possible to do better than that.

Specifically, a PyGILState_SwitchInterpreter API could focus solely on the manipulation of the "active thread state" TLS key entry. The sequence of commands in mod_wsgi would then look like:

old_interp = PyGILState_SwitchInterpreter(target_interp);
old_gil = PyGILState_Ensure();
/* Call into Python using target_interp */
PyGILState_Release(old_gil);
PyGILState_SwitchInterpreter(old_interp); /* May not be needed in the mod_wsgi case, since it knows it is making the outermost call into the PyGILState_* APIs */

All of the other elements of Antoine's proposal (i.e. the per-interpreter TLS key entries) would still be needed, it's just that the API for associating a thread with an interpreter would remain separated from that of associating the thread with a particular thread state.

The big advantage of doing it this way is that it will nest properly, whereas PyGILState_EnsureEx would need a more complicated API to correctly report both the old interpreter state and the old GIL state within that interpreter.
msg169286 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-28 16:47
Le mardi 28 août 2012 à 14:12 +0000, Nick Coghlan a écrit :
> old_interp = PyGILState_SwitchInterpreter(target_interp);
> old_gil = PyGILState_Ensure();
> /* Call into Python using target_interp */
> PyGILState_Release(old_gil);
> PyGILState_SwitchInterpreter(old_interp); /* May not be needed in the mod_wsgi case, since it knows it is making the outermost call into the PyGILState_* APIs */

Why wouldn't it be simply written:

old_gil = PyGILState_EnsureEx(target_interp);
/* Call into Python using target_interp */
PyGILState_Release(old_gil);
msg169310 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-28 22:10
Sorry, Mark. Is not for associating thread state specified by embedding application. In simple terms it is exactly like existing PyGILState_Ensure() in that caller doesn't have a thread state, whether it has already been created. Only difference is to allow that simplified API to work against a sub interpreter.

Nick, I previously said:

"""In the case of the latter, if a Python thread is created in a specific sub interpreter, it should automatically designate for that thread that that is its interpreter context, so if it calls out and does the Release/Ensure dance, that it goes back into the same sub interpreter."""

So yes to your:

"""Thinking about it, I believe there still needs to be a concept of an "active thread state" TLS key in order to deal with Graham's original problem. Specifically, if PyGILState_EnsureEx is used to associate the thread with a particular interpreter, then subsequent calls to PyGILState_Ensure from *that thread* should get the explicitly associated interpreter, rather than the main interpreter."""

My example was more to do with a thread created in Python then calling out and back in, but same deal as foreign thread calling in, out and back in.

Antoine, yes, can possibly can be simplified to that. The original idea of a switch interpreter function was suggested on basis that PyGILState_Ensure would not be modified or extended version of function created. Rolling an implicit switch interpreter into PyGILState_EnsureEx when argument is different to the current may serve same purpose.
msg169329 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-29 02:19
The reason I'm proposing going back to the original SwitchInterpreter idea is because the EnsureEx idea doesn't nest cleanly - if your thread is already associated with "interpreter A", you can't readily call into "interpeter B", because the API provides no way to correctly restore the associated interpreter back to interpreter A when you're done.

EnsureEx works fine if extension modules are not aware of multiple interpreters:

   /* Embedding application (GIL always unlocked) */
   gilstate = PyGILState_EnsureEx(interp_A);
     /* Python code and extension code happens */
       /* Callback needs to reach back into Python */
       cb_gilstate = PyGILState_Ensure();
         /* Callback runs */
       PyGILState_Release(cb_gilstate);
     /* Python code and extension code unwinds */
   PyGILState_Release(gilstate);

However, it *doesn't* work (at least, not easily) if the extension itself wants to call back into an interpreter other than the one already associated with the current thread:

   /* Embedding application (GIL always unlocked) */
   gilstate = PyGILState_EnsureEx(interp_A);
     /* Python code and extension code happens */
       /* Callback needs to reach back into a specific interpreter */
       cb_gilstate = PyGILState_EnsureEx(interp_B);
         /* Callback runs */
       PyGILState_Release(cb_gilstate);
     /* Python code and extension code unwinds */
   PyGILState_Release(gilstate);

Does that second call to EnsureEx fail? If it succeeds, how does the client know which interpreter to use for the PyGILState_Release call? It could be made to work if PyGILState_STATE was changed from an enum to a struct that included in interpreter state pointer, or if EnsureEx returned a different type and was paired up with a new ReleaseEx pointer.

However, that's starting to get very messy compared to a separate SwitchInterpreter call:

   /* Embedding application (GIL always unlocked) */
   old_interp = PyGILState_SwitchInterpreter(interp_A);
   /* "autoTLSkey" now refers to a thread state for interpreter A */
   gilstate = PyGILState_Ensure();
     /* Python code and extension code happens */
       /* Callback needs to reach back into Python */
       pre_cb_interp = PyGILState_SwitchInterpreter(interp_B);
       /* "autoTLSkey" now refers to a thread state for interpreter B */
       cb_gilstate = PyGILState_Ensure();
         /* Callback runs */
       PyGILState_Release(cb_gilstate);
       PyGILState_SwitchInterpreter(pre_cb_interp);
       /* "autoTLSkey" again refers to a thread state for interpreter A */
     /* Python code and extension code unwinds */
   PyGILState_Release(gilstate);
   PyGILState_SwitchInterpreter(old_interp);

And yes, I'm pondering ways that this could be used to implement Rust-style "channels" for communication between interpreters without needing to copy data, by using this API to create proxy interfaces for accessing an object owned by another subinterpreter.
msg169330 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-29 02:49
Nick. Valid point.

I guess I hadn't been thinking about case of one thread calling out of one interpreter and then into another, as I don't do it in mod_wsgi and even I regard doing that as partly evil.

Does that mean this switch interpreter call somehow gets used in the Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS.
msg169332 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-29 04:03
No, those wouldn't change at all. However, after thinking about it further, I'm leaning back towards the option of an EnsureEx/ReleaseEx pair that allows the previous interpreter to be reported and restored along with the GIL state. I'd also like to make the state an opaque structure that isn't exposed in the limited API. Something along the lines of:

    typedef struct _gilinfo {
        PyGILState_STATE  lock_state;
        PyInterpreter     *active;
    } PyGILState_INFO;

    int /* Allow returning an error code */
    PyGILState_EnsureEx(PyInterpreterState *target,
                        PyGILState_INFO    *prev);
                                        
    void
    PyGILState_ReleaseEx(PyGILState_INFO *prev);

This allows various issues related to implicitly creating thread states to be handled the same way they are now, where PyThreadState_New will create a persistent thread state, while PyGILState_EnsureEx will either use a preexisting thread state for the requested interpreter (if it exists), or implicitly create one. Implicitly created thread states would be destroyed by the corresponding call to ReleaseEx.

To make this work, I believe all that should be needed is for:

1. PyInterpreterState updated to include a per-interpreter TLS key
2. _PyGILState_NoteThreadState would update both the autoTLSkey entry and the per-interpreter key entry
3. PyGILState_EnsureEx added to support switching the autoTLSkey entry to point to a different thread state (creating it if needed)
4. PyGILState_Ensure updated to map to PyGILState_EnsureEx(NULL) and to extract the lock state from the info structure
5. PyGILState_Release updated to populate the info structure with the active interpreter and the passed in previous GIL state before calling PyGILState_ReleaseEx
5. PyThreadState_Delete and PyThreadState_DeleteCurrent updated to also clobber the per-interpreter TLS key entry

PyThreadState_New already calls _PyGILState_NoteThreadState implicitly, so Python created threads (and embedded threads with an explicitly created thread state) should do the right thing automatically.

Making the INFO struct an opaque token also means it can be expanded to cover any future needs that arise.
msg169333 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-29 04:13
So you are saying that as user of this I am meant to call it as:

PyGILState_INFO info;

PyGILState_EnsureEx(interp, &info);
...
PyGILState_ReleaseEx(&info);

What is the potential error code from PyGILState_EnsureEx() considering that right now for PyGILState_Ensure() it is a value passed back into PyGILState_Release().
msg169334 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-29 04:21
Currently, both PyGILState_Ensure and PyGILState_Release just call PyFatal_Error if anything goes wrong. With the *Ex versions, it would be possible to replace those fatal errors with more conventional error handling.

For Ensure, the likely error is failing to create the thread state.

For Release, the likely error is calling it with the system in the wrong state.

I think the current API reflects a difference between an extension mindset and an embedding mindset. If those calls go wrong in an extension context, killing the entire application is likely the only thing you can reasonably do. In an embedded context, though, the embedding application likely has other options (such as reinitialising the entire Python interpreter).
msg169352 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-29 09:12
Le mercredi 29 août 2012 à 02:19 +0000, Nick Coghlan a écrit :
> However, it *doesn't* work (at least, not easily) if the extension
> itself wants to call back into an interpreter other than the one
> already associated with the current thread:
> 
>    /* Embedding application (GIL always unlocked) */
>    gilstate = PyGILState_EnsureEx(interp_A);
>      /* Python code and extension code happens */
>        /* Callback needs to reach back into a specific interpreter */
>        cb_gilstate = PyGILState_EnsureEx(interp_B);
>          /* Callback runs */
>        PyGILState_Release(cb_gilstate);
>      /* Python code and extension code unwinds */
>    PyGILState_Release(gilstate);
> 
> Does that second call to EnsureEx fail?

What would it fail? It should simply change the thread state to
interp_B's thread state for the current thread. Then the nested
PyGILState_Release changes the thread state back to its old value.

(of course, I don't understand how an extension running from a given
interpreter would have access to another interpreter's callback, but
regardless, it's technically not a problem)

> If it succeeds, how does the client know which interpreter to use for
> the PyGILState_Release call? It could be made to work if
> PyGILState_STATE was changed from an enum to a struct that included in
> interpreter state pointer, or if EnsureEx returned a different type
> and was paired up with a new ReleaseEx pointer.

Making PyGILState_STATE a struct sounds reasonable to me.

> However, that's starting to get very messy compared to a separate
> SwitchInterpreter call:

Why messy?
msg169354 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-29 09:31
Yeah, I eventually came around to agreeing that it's better to stick with the Ensure/Release model. SwitchInterpreter gets messy when it comes to managing the lifecycle of temporary thread states.

So an EnsureEx/ReleaseEx pair with a new struct that includes both the GIL state info and either the previous thread state or an interpreter pointer sounds good to me.
msg169359 - (view) Author: Graham Dumpleton (grahamd) Date: 2012-08-29 10:09
If PyGILState_STATE is a struct, what happens if someone naively does:

PyGILState_Release(PyGILState_UNLOCKED)

I know they shouldn't, but I actually do this in mod_wsgi in one spot as it is otherwise a pain to carry around the state when I know for sure if was unlocked before the PyGILState_Ensure().

Or can PyGILState_UNLOCKED map to some a global struct instance with certain state in it that represents that without problem.
msg198115 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-19 20:40
I wanted to set issue10915 as duplicate but there is actually a tentative patch there. Unfortunately the discussions are now split apart...
msg206016 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 02:38
FYI I fixed a weird behaviour of the PyThread_set_key_value() function. The change has indirectly on impact on test.support.run_in_subinterp():
http://bugs.python.org/issue19787#msg206015

So my change might have an effect on this issue.
msg206017 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 02:40
They are now two issues (#10915 and this one) with many messages. Both issues are open. Can someone please make a summary? How can we fix the GIL/subinterpreter issue?
msg206042 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-13 10:14
New changeset 5d078b0bae75 by Victor Stinner in branch 'default':
Issue #19787: PyThread_set_key_value() now always set the value
http://hg.python.org/cpython/rev/5d078b0bae75
msg206048 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 10:23
> FYI I fixed a weird behaviour of the PyThread_set_key_value() function. > The change has indirectly on impact on test.support.run_in_subinterp():

The impact was a bug. I reverted my changeset and pushed a new changeset which leaves _PyGILState_NoteThreadState() unchanged.
msg261779 - (view) Author: Roundup Robot (python-dev) Date: 2016-03-14 21:50
New changeset e590c632c9fa by Victor Stinner in branch 'default':
Add more checks on the GIL
https://hg.python.org/cpython/rev/e590c632c9fa
History
Date User Action Args
2016-03-14 21:50:02python-devsetmessages: + msg261779
2015-07-03 00:31:46eric.snowsetversions: + Python 3.6, - Python 3.4
2013-12-13 10:23:22hayposetmessages: + msg206048
2013-12-13 10:14:20python-devsetnosy: + python-dev
messages: + msg206042
2013-12-13 02:40:26hayposetmessages: + msg206017
2013-12-13 02:38:23hayposetmessages: + msg206016
2013-09-19 20:44:19hayposetnosy: + haypo
2013-09-19 20:40:35pitrousetmessages: + msg198115
2012-11-13 05:26:25eric.snowsetnosy: + eric.snow
2012-09-10 02:12:14jceasetnosy: + jcea
2012-08-29 10:09:44grahamdsetmessages: + msg169359
2012-08-29 09:31:37ncoghlansetmessages: + msg169354
2012-08-29 09:12:36pitrousetmessages: + msg169352
2012-08-29 04:21:18ncoghlansetmessages: + msg169334
2012-08-29 04:13:59grahamdsetmessages: + msg169333
2012-08-29 04:03:39ncoghlansetmessages: + msg169332
2012-08-29 02:49:48grahamdsetmessages: + msg169330
2012-08-29 02:19:13ncoghlansetmessages: + msg169329
2012-08-28 22:10:12grahamdsetmessages: + msg169310
2012-08-28 16:47:57pitrousetmessages: + msg169286
2012-08-28 14:12:10ncoghlansetmessages: + msg169284
2012-08-28 13:36:23mhammondsetmessages: + msg169280
2012-08-28 13:21:28mhammondsetnosy: + mhammond
messages: + msg169278
2012-08-24 13:00:28grahamdsetmessages: + msg169014
2012-08-24 12:55:42ncoghlansetmessages: + msg169013
title: Add PyGILState_SwitchInterpreter -> Support subinterpreters in the GIL state API
2012-08-24 12:45:06pitrousetmessages: + msg169010
2012-08-24 12:35:56ncoghlansetmessages: + msg169008
2012-08-24 12:06:14asvetlovsetnosy: + asvetlov
2012-08-22 00:46:30grahamdsetmessages: + msg168832
2012-08-21 22:56:45pitrousetmessages: + msg168818
2012-08-21 22:36:40grahamdsetmessages: + msg168816
2012-08-21 22:31:53pitrousetmessages: + msg168815
2012-08-21 22:14:32grahamdsetmessages: + msg168812
2012-08-21 16:27:32pitrousetmessages: + msg168792
2012-08-21 16:08:02ncoghlansetmessages: + msg168790
2012-08-21 15:51:24pitrousetmessages: + msg168789
2012-08-21 15:23:56ncoghlansetmessages: + msg168787
2012-08-21 14:47:26pitrousetmessages: + msg168780
2012-08-21 14:44:03ncoghlansetmessages: + msg168779
2012-08-21 14:33:43ncoghlansetmessages: + msg168776
2012-08-21 14:10:23pitrousetmessages: + msg168772
2012-08-21 14:06:08ncoghlansetmessages: + msg168771
2012-08-21 13:28:07pitrousetmessages: + msg168769
2012-08-21 13:25:51ncoghlansetmessages: + msg168768
2012-08-21 13:21:29ncoghlansetmessages: + msg168766
2012-08-21 12:56:21pitrousetnosy: + pitrou
messages: + msg168764
2012-08-21 11:56:34grahamdsetnosy: + grahamd
messages: + msg168759
2012-08-21 05:18:00ncoghlancreate