Repository navigation
Add an id field to PyInterpreterState. #73288
Description
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 = 'https://github.com/ericsnowcurrently'
closed_at = <Date 2017-05-25.00:22:01.977>
created_at = <Date 2016-12-29.06:53:42.384>
labels = ['interpreter-core', 'type-feature', '3.7']
title = 'Add an id field to PyInterpreterState.'
updated_at = <Date 2017-05-25.00:22:01.976>
user = 'https://github.com/ericsnowcurrently'bugs.python.org fields:
activity = <Date 2017-05-25.00:22:01.976>
actor = 'eric.snow'
assignee = 'eric.snow'
closed = True
closed_date = <Date 2017-05-25.00:22:01.977>
closer = 'eric.snow'
components = ['Interpreter Core']
creation = <Date 2016-12-29.06:53:42.384>
creator = 'eric.snow'
dependencies = []
files = ['46070', '46094', '46100', '46105']
hgrepos = []
issue_num = 29102
keywords = ['patch']
message_count = 32.0
messages = ['284229', '284230', '284232', '284233', '284273', '284276', '284278', '284280', '284283', '284290', '284328', '284329', '284332', '284335', '284350', '284353', '284359', '284366', '284374', '284378', '284380', '284395', '284406', '293899', '294116', '294212', '294232', '294312', '294315', '294370', '294371', '294417']
nosy_count = 7.0
nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'grahamd', 'eric.snow', 'serhiy.storchaka', 'steve.dower']
pr_nums = ['1639']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue29102'
versions = ['Python 3.7']
Activity
ericsnowcurrently commentedon Dec 29, 2016
Currently there isn't any way to uniquely identify an interpreter. This patch adds a new "id" field to the PyInterpreterState struct. The ID for every new interpreter is set to the value of an increasing global counter. That means that the ID is unique within the process.
IIRC, the availability of unique ID would help tools that make use of subinterpreters, like mod_wsgi. It is also necessary for any effort to expose interpreters in Python-level code (which is the subject of other ongoing work).
The patch also adds:
unsigned long PyInterpreterState_GetID(PyInterpreterState *interp)
Note that, without a Python-level interpreters module, testing this change is limited to extending the existing test code in test_capi.
serhiy-storchaka commentedon Dec 29, 2016
Why not use just the pointer to PyInterpreterState itself?
ericsnowcurrently commentedon Dec 29, 2016
Pointers can get re-used, so they aren't temporally unique.
serhiy-storchaka commentedon Dec 29, 2016
What is the use case of keeping the uniqueness after deleting an interpreter?
zooba commentedon Dec 29, 2016
Tracking purposes mainly, so someone outside the interpreter state can tell when it's no longer there. Making interpreter states weak-referencable would have a similar effect, and could very well use this id if we didn't need the callback.
serhiy-storchaka commentedon Dec 29, 2016
If add an API for getting an unique ID of the interpreter state, is it needed to add an API for getting the interpreter state by ID?
ericsnowcurrently commentedon Dec 29, 2016
Three reasons come to mind:
Since PyInterpreterState is not a PyObject, using weakrefs to address the third point won't work, right?
serhiy-storchaka commentedon Dec 29, 2016
There is an issue with integer identifiers of threads. See bpo-25658 and https://mail.python.org/pipermail/python-ideas/2016-December/043983.html.
zooba commentedon Dec 29, 2016
That's an issue with TLS initialisation, not thread IDs. It's easily solved by defining an "uninitialized" value (e.g. 0) and an "invalid" value (e.g. -1).
Interpreter states are in a linked list, so you can traverse the list to find one by ID.
WRT weakrefs, we can't use them directly, but I suspect the higher-level API will need it. Possibly adding a callback on finalisation would fill both needs, but I like having a reliable ID - otherwise we'll probably end up with multiple different IDs managed indirectly via callbacks. (Perhaps a single callback for when any interpreter is finalized that passes the ID through? That should work well, since the ID is designed to outlive the interpreter itself, so it can be an asynchronous notification.)
ericsnowcurrently commentedon Dec 29, 2016
Interpreter states are in a linked list, so you
can traverse the list to find one by ID.
Exactly. At first I had added a PyInterpreterState_FindByID() or something
like that. However, as you noted, I realized it wasn't necessary. :)
WRT weakrefs, we can't use them directly, but I suspect the higher-level
API will need it...
Everything you said about weakrefs sounds good. We can discuss more when
we get to that high-level API.
ncoghlan commentedon Dec 30, 2016
+1 from me for the general idea.
One subtlety with the draft implementation is that an Initialize/Finalize cycle doesn't reset the counter, which:
What do you think about resetting the counter back to 1 in Py_Initialize?
10 remaining items
vstinner commentedon Dec 31, 2016
I would prefer to not use "fast" C types because they are not well supported. For example, ctypes has ctypes.c_int64 but no ctypes.c_int_fast64.
Previous work adding an unique identifier: PEP-509
https://www.python.org/dev/peps/pep-0509/#integer-overflow
ericsnowcurrently commentedon Dec 31, 2016
Thanks for pointing that out, Victor. Given the precedent I switched to using int64_t. The patch actually uses PY_INT64_T, but I didn't see a reason to use int64_t directly. FWIW, there *are* a few places that use int_fast64_t, but they are rather specialized and I didn't want this patch to be a place where I had to deal with setting a more general precedent. :)
vstinner commentedon May 17, 2017
What the status of this issue Eric? Do you still need interpreter ID?
ericsnowcurrently commentedon May 21, 2017
Yes, I still need it. :)
ericsnowcurrently commentedon May 23, 2017
New changeset e377416 by Eric Snow in branch 'master':
bpo-29102: Add a unique ID to PyInterpreterState. (bpo-1639)
e377416
serhiy-storchaka commentedon May 23, 2017
This change added a compiler warning.
./Programs/_testembed.c: In function ‘print_subinterp’:
./Programs/_testembed.c:31:22: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘int64_t {aka long long int}’ [-Wformat=]
printf("interp %lu <0x%" PRIXPTR ">, thread state <0x%" PRIXPTR ">: ",
^
ericsnowcurrently commentedon May 24, 2017
Thanks for pointing this out, Serhiy. I'll take a look in the morning.
vstinner commentedon May 24, 2017
Does someone know the PRxxx constant for int64_t?
ericsnowcurrently commentedon May 24, 2017
Apparently it is PRId64.
ericsnowcurrently commentedon May 24, 2017
(see bpo-30447)
ericsnowcurrently commentedon May 25, 2017
I've fixed the compiler warning via d1c3c13.