classification
Title: Provide convenient C API for storing per-interpreter state
Type: enhancement Stage: resolved
Components: Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: arigo, eric.snow, ncoghlan, petr.viktorin, vstinner
Priority: normal Keywords: patch

Created on 2019-02-26 13:46 by ncoghlan, last changed 2019-03-15 23:49 by eric.snow. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12132 merged eric.snow, 2019-03-01 20:50
Messages (9)
msg336670 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-02-26 13:46
(New issue derived from https://bugs.python.org/issue35886#msg336501 )

cffi needs a generally available way to get access to a caching dict for the currently active subinterpreter. Currently, they do that by storing it as an attribute in the builtins namespace: https://bitbucket.org/cffi/cffi/src/07d1803cb17b230571e3155e52082a356b31d44c/c/call_python.c?fileviewer=file-view-default

As a result, they had to amend their code to include the CPython internal headers in 3.8.x, in order to regain access to the "builtins" reference.

Armin suggested that a nicer way for them to achieve the same end result is if there was a PyInterpreter_GetDict() API, akin to https://docs.python.org/3/c-api/init.html#c.PyThreadState_GetDict

That way they could store their cache dict in there in 3.8+, and only use the builtin dict on older Python versions.
msg336695 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-26 16:54
+1 from me

@Armin, thanks to Nick I understand your request better now.  I'll put up a PR by the end of the week if no one beats me to it.
msg336950 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-03-01 20:05
Thinking about this, what is the key difference with the existing PyModule_GetState() function?  Is it just the return type (module-defined void * vs. a regular dict)?  Certainly it provides a C-only namespace that all extensions can share (like PyThreadState_Get() does), but I'm not sure that's desirable.

Anyway, I'd rather not add PyInterpreterState_GetDict() if it is essentially equivalent to PyModule_GetState().
msg336997 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2019-03-02 07:33
PyModule_GetState() requires having the module object that corresponds to the given interpreter state.  I'm not sure how a C extension module is supposed to get its own module object corresponding to the current interpreter state, without getting it from the caller in some way.

The mess in cffi's call_python.c would be much reduced if we had bpo-36124 (fixed to call Py_CLEAR(), see comment in bpo-36124).

If you want to point out a different approach that might work too, that's OK too.  It's just that the current approach was arrived at after multiple generations of crash reports, which makes me uneasy about changing it in more subtle ways than just killing it in favor of a careful PyInterpreterState_GetDict().

If you want to see details of the current hacks, I can explain https://bitbucket.org/cffi/cffi/src/d765c36df047cf9d5e766777049c4107e1f4cb00/c/call_python.c :

The goal is that we are given a finite (but unknown at compile-time) number of 'externpy' data structures, and for each pair (externpy, interp) the user can assign a callable 'PyObject *'.  The annoying part of the logic is that we have a C-exposed callback function (line 204) which is called with a pointer to one of these 'externpy' structures, and we need to look up the right 'PyObject *' to call.

At line 255 we just got the GIL and need to check if the 'PyThreadState_GET()->interp' is equal to the one previously seen (an essential optimization: we can't do complicated logic in the fast path).  We hack by checking for 'interp->modules' because that's a PyObject.  The previous time this code was invoked, we stored a reference to 'interp->modules' in the C structure 'externpy', with an incref.  So this fast-path pointer comparison is always safe (no freed object whose address can be accidentally reused).  This test will quickly pass if this function is called in the same 'interp' many times in a row.

The slow path is in _update_cache_to_call_python(), which calls _get_interpstate_dict(), whose only purpose is to return a dictionary that depends on 'interp'.  Note how we need to be very careful about various cases, like shutdown.  _get_interpstate_dict() can fail and return NULL, but it cannot give a fatal error.  That's why we couldn't call, say, PyImport_GetModuleDict(), because this gives a fatal error if 'interp' is being shut down at the moment.

Overall, the logic uses both 'interp->modules' and 'interp->builtins'.  The 'modules' is used only for the pointer equality check, because that's an object that is not supposed to be freed until the very last moment.  The 'builtins' is used to store the special name "__cffi_backend_extern_py" in it, because we can't store that in 'interp->modules' directly without crashing various 3rd-party Python code if this special key shows up in 'sys.modules'.  The value corresponding to this special name is a dictionary {PyLong_FromVoidPtr(externpy): infotuple-describing-the-final-callable}.
msg337089 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-03-04 10:42
PyModule_GetState() gives you *per-module* state, not per-interpreter state.

Module objects are shared across subinterpreters, unless you use multi-phase initialization.

> PyModule_GetState() requires having the module object that corresponds
> to the given interpreter state.  I'm not sure how a C extension module
> is supposed to get its own module object corresponding to the current
> interpreter state, without getting it from the caller in some way.

This is the problem described in PEP 573: you don't always have access to your own module object. That keeps some more complex modules from switching to multi-phase init.

Unless this issue can wait for when PEP 580, PEP 573, and possibly some fallout of unknown unknowns are solved, let's add PyInterpreterState_GetDict for now.
msg337501 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-03-08 16:12
On Sat, Mar 2, 2019 at 12:33 AM Armin Rigo <report@bugs.python.org> wrote:
> PyModule_GetState() requires having the module object that corresponds
> to the given interpreter state.  I'm not sure how a C extension module is
> supposed to get its own module object corresponding to the current
> interpreter state, without getting it from the caller in some way.

Fair enough. :)

> If you want to point out a different approach that might work too, that's OK too.

As Petr noted, the preferred solution isn't feasible yet (pending
several PEPs) and depends on using multi-phase extension module
initialization (PEP 489).  Furthermore, that assumes that the
preferred solution would meet your performance needs.  If you think it
wouldn't then this is a great chance to speak up. :)

> It's just that the current approach was arrived at after multiple generations of
> crash reports, which makes me uneasy about changing it in more subtle ways
> than just killing it in favor of a careful PyInterpreterState_GetDict().

Understood.

Thanks for the detailed explanation on why you are using
"interp->dict", and how you need to avoid fatal errors (e.g. from
"PyImport_GetModuleDict()" during shutdown).

Your solution seems reasonable, since every interpreter will have it's
own "modules" object.  However, note that "interp->modules" can get
swapped out with a different object at any moment.  The use case is
temporarily setting a different import state (e.g. isolating a
module's import).  Currently this isn't very common (especially since
"interp->modules" is currently not sync'ed with "sys.modules"), but we
have plans for making this easier to do from Python code in the
not-distant future.

Regardless, I agree that PyInterpreterState_GetDict() will solve
several problems for you.  I'm sorry we didn't provide this solution
for you sooner.
msg337506 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-03-08 16:17
Also, while PyThreadState_GetDict() is the inspiration here, we don't have to copy it exactly.  For instance, PyInterpreterState_GetDict() takes a PyInterpreterState* argument, whereas PyThreadState_GetDict() takes no arguments and gets the PyThreadState* from thread-local storage.

Is there anything else that would make sense to do differently with PyInterpreterState_GetDict()?  It's pretty basic, so I'm guessing "no". :)
msg338043 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-03-15 23:47
New changeset d2fdd1fedf6b9dc785cf5025b548a989faed089a by Eric Snow in branch 'master':
bpo-36124: Add PyInterpreterState.dict. (gh-12132)
https://github.com/python/cpython/commit/d2fdd1fedf6b9dc785cf5025b548a989faed089a
msg338044 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-03-15 23:49
@arigo, thanks for nudging us here. :)  Let me know if there's anything else that would help here.
History
Date User Action Args
2019-03-15 23:49:00eric.snowsetstatus: open -> closed
resolution: fixed
messages: + msg338044

stage: patch review -> resolved
2019-03-15 23:47:48eric.snowsetmessages: + msg338043
2019-03-08 16:17:29eric.snowsetmessages: + msg337506
2019-03-08 16:12:57eric.snowsetmessages: + msg337501
2019-03-04 10:42:46vstinnersetnosy: + vstinner
2019-03-04 10:42:02petr.viktorinsetmessages: + msg337089
2019-03-02 07:33:48arigosetmessages: + msg336997
2019-03-01 20:52:37eric.snowsetassignee: eric.snow
2019-03-01 20:50:51eric.snowsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request12135
2019-03-01 20:37:09eric.snowsetnosy: + petr.viktorin
2019-03-01 20:05:13eric.snowsetmessages: + msg336950
2019-02-26 16:54:29eric.snowsetnosy: + arigo, eric.snow
messages: + msg336695
2019-02-26 13:47:32ncoghlansetstage: needs patch
type: enhancement
versions: + Python 3.8
2019-02-26 13:46:42ncoghlancreate