[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

I've fixed the compiler warning via d1c3c13fedaf62b71445ccd048e395aa4a7d510f.

--
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

(see issue30447)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

Apparently it is PRId64.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[STINNER Victor]

STINNER Victor added the comment:

Does someone know the PRxxx constant for int64_t?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

Thanks for pointing this out, Serhiy.  I'll take a look in the morning.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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 ">: ",
  ^

--
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Changes by Eric Snow :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:


New changeset e377416c10eb0bf055b0728cdcdc4488fdfd3b5f by Eric Snow in branch 
'master':
bpo-29102: Add a unique ID to PyInterpreterState. (#1639)
https://github.com/python/cpython/commit/e377416c10eb0bf055b0728cdcdc4488fdfd3b5f


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

Yes, I still need it. :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[STINNER Victor]

STINNER Victor added the comment:

What the status of this issue Eric? Do you still need interpreter ID?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Changes by Eric Snow :


--
pull_requests: +1734

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

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. :)

--
Added file: http://bugs.python.org/file46105/interpreter-id-4.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[STINNER Victor]

STINNER Victor added the comment:

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

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

I've updated the patch to address Nick's review.  Thanks!

--
Added file: http://bugs.python.org/file46100/interpreter-id-3.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

int_fast64_t it is then. :)  I vacillated between the options and went with the 
bigger space.  However, you're right that following convention is worth it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Nick Coghlan]

Nick Coghlan added the comment:

Yeah, I'm also fine with using int_fast64_t for the subinterpreter count.

The only thing I'm really advocating for strongly on that front is that I think 
it makes sense to sacrifice the sign bit in the ID field as an error indicator 
that provides a more idiomatic C API.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Steve Dower]

Steve Dower added the comment:

I fully expect subinterpreters to have a serious role in long running 
applications like web servers or other agents (e.g. cluster nodes), so I'd 
rather just bite the bullet and take 64-bits now so that we can completely 
neglect reuse issues. Otherwise we'll find ourselves adding infrastructure to 
hide the fact that you may see the same id twice.

Another four bytes is a cheap way to avoid an entire abstraction layer.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Nick Coghlan]

Nick Coghlan added the comment:

The concern I have with using an unsigned value as the interpreter ID is that 
it's applying the "NULL means an error" idiom or the "false means an error" 
idiom to a non-pointer and non-boolean return type, whereas the common 
conventions for integer return values are:

* 0 = success in CLI return codes
* non-negative = success in int-based C APIs

If we were to use int_fast32_t for IDs instead, then any negative value can 
indicate an error, and the main interpreter could be given ID 0 to better align 
with the threading.Thread naming scheme.

Whether we hit runtime error at 2 billion subinterpreters or 4 billion 
subinterpreters in a single process isn't likely to make much difference to 
anyone, but choosing an idiosyncratic error indicator will impact everyone that 
attempts to interact with the API.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

Here's the updated patch.

--
Added file: http://bugs.python.org/file46094/interpreter-id-2.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

> What do you think about resetting the counter back to 1 in Py_Initialize?

Sounds good to me.  When I was working on the patch I had the idea in the back 
of my mind that not resetting the counter would better support interpreter 
separation efforts in the future.  However, after giving it some thought I 
don't think that's the case.  So resetting it in Py_Initialize() is fine with 
me.

> I'll also note that in the threading module, the main thread is
> implicitly thread 0 (but named as MainThread) - Thread-1 is the first
> thread created via threading.Thread. So it may make sense to use a
> signed numeric ID, with 0 being the main interpreter, 1 being the first
> subinterpreter, and negative IDs being errors.

I had considered that and went with an unsigned long.  0 is used for errors, 
and starting at 1, which effectively means the main interpreter is always 1.  
If we later run into overflow issues then we can sort that out at that point 
(e.g. by moving to a 64-bit int or even a Python int).

I'll add comments to the patch regarding these points.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Steve Dower]

Steve Dower added the comment:

> Wouldn't this break the main property of IDs, the uniqueness?

If we bump it up to a 64-bit ID then it'll be no worse than how we track all 
dict mutations.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Nick Coghlan]

Nick Coghlan added the comment:

It depends on the scope of uniqueness we're after. `threading._counter()` 
(which is the small-integer-ID debugging counter for threading.Thread names) is 
a module global in the threading module, so an Initialize/Finalize cycle will 
reset it.

If we wanted to track "Which Initialize/Finalize cycle is this?" *as well*, it 
would make more sense to me to have that as a separate "runtime" counter, such 
that the full coordinates of the current point of execution were:

- runtime counter (How many times has Py_Initialize been called?)
- interpreter counter (Which interpreter is currently active?)
- thread name (Which thread is currently active?)

I'll also note that in the threading module, the main thread is implicitly 
thread 0 (but named as MainThread) - Thread-1 is the first thread created via 
threading.Thread. So it may make sense to use a signed numeric ID, with 0 being 
the main interpreter, 1 being the first subinterpreter, and negative IDs being 
errors.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> What do you think about resetting the counter back to 1 in Py_Initialize?

Wouldn't this break the main property of IDs, the uniqueness?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Nick Coghlan]

Nick Coghlan added the comment:

+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:

1. Increases the chance of counter overflow (while admittedly still leaving it 
incredibly low)
2. Means you still can't readily check whether the current interpreter is the 
main interpreter (i.e. the one created automatically in Py_Initialize)

What do you think about resetting the counter back to 1 in Py_Initialize?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Steve Dower]

Steve Dower added the comment:

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.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

There is an issue with integer identifiers of threads. See issue25658 and 
https://mail.python.org/pipermail/python-ideas/2016-December/043983.html.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

Three reasons come to mind:

1. threads are identified by small integers
2. long, random-looking IDs are not human-friendly, and subinterpreter IDs will 
be used like thread IDs are
3. related to what Steve said, temporally unique IDs allow us to be confident 
about whether or not an interpreter has been destroyed (and how many 
interpreters there have been)

Since PyInterpreterState is not a PyObject, using weakrefs to address the third 
point won't work, right?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Steve Dower]

Steve Dower added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

What is the use case of keeping the uniqueness after deleting an interpreter?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

Eric Snow added the comment:

Pointers can get re-used, so they aren't temporally unique.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Why not use just the pointer to PyInterpreterState itself?

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29102] Add an id field to PyInterpreterState.

[Eric Snow]

New submission from Eric Snow:

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.

--
assignee: eric.snow
components: Interpreter Core
files: interpreter-id.diff
keywords: patch
messages: 284229
nosy: brett.cannon, eric.snow, grahamd, ncoghlan, steve.dower
priority: normal
severity: normal
stage: patch review
status: open
title: Add an id field to PyInterpreterState.
type: enhancement
versions: Python 3.7
Added file: http://bugs.python.org/file46070/interpreter-id.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com