PEP 788: Address feedback from first discussion round #4400

ZeroIntensity · 2025-05-03T15:46:12Z

ZeroIntensity · 2025-05-04T17:56:14Z

ZeroIntensity · 2025-05-08T10:13:04Z

peps/pep-0788.rst

vstinner · 2025-05-09T15:39:01Z

vstinner

peps/pep-0788.rst

colesbury

peps/pep-0788.rst

colesbury · 2025-05-12T21:55:53Z

peps/pep-0788.rst

+  deallocated, and shutdown for the main interpreter includes the entire Python
+  runtime being finalized.
+
+Native and Python Threads


"Native threads" is not a good term for non-threading module created threads even if you (re)define the term. It perpetuates a confusion that Python threads aren't "real" OS threads.

We use "non-Python created threads" in, e.g., https://docs.python.org/3/c-api/init.html#non-python-created-threads.

I don't totally disagree, but I'd really prefer not using the clunky phrase "non-Python created thread", especially considering how many times "native thread" is used throughout the PEP. Is there a shorter term we could use?

OS threads?

I think "OS thread" has the same problem as "native thread". I'm personally fine with still using the term "native thread", because this PEP will change how threading works too. So you get two interpretations, both being correct:

"Reimagining Threads natively": The PEP changes how to use OS threads with a native caller.

"Reimagining Native Threads": The PEP changes how native OS threads, threading included, interact with the interpreter.

colesbury · 2025-05-12T21:58:01Z

peps/pep-0788.rst

-like this:
-
-.. code-block:: c
+In Python, threads are able to interact with an interpreter (e.g., invoke the


I don't think this paragraph is helpful here. Get to the crux: PyGILState_Ensure/PyGILState_Release is one of the primary ways to get a valid thread state using the C API and it doesn't work well with subinterpreters because it always creates (or reuses) a thread state for the main interpreter.

I think it's useful as a quick intro to people who aren't deeply familiar with how thread states work. I would expect that 99% of extension developers just know the PyGILState surface API, so the thread state terminology that the proposal uses might seem foreign. Would you prefer I just link to the documentation?

I think talking about terminology is okay, but it doesn't need to be at the beginning of the abstract. When reading the abstract, I'd want to see:

The most important problem the PEP is solving

An outline of the solution without going in to too much detail

colesbury · 2025-05-12T21:59:07Z

peps/pep-0788.rst

+called. There's a subtle difference between the two terms, as used in this
+PEP:
+
+- "Finalization" refers to an interpreter getting ready to "shut down", in


I don't understand the distinction being made here between finalization and shutdown nor the purpose of making this distinction -- I don't see how "interpreter finalization" and "interpreter shutdown" are different things.

"Finalization" is also pretty overloaded; you can say "interpreter finalization" to be more clear.

It's not important for the main interpreter, but there's an important distinction for subinterpreters because the PyInterpreterState structure becomes invalid after shutdown, but not during finalization. That's an important detail for the rationale on why we're not using PyInterpreterState * for PyThreadState_Ensure.

But I think you're right, anyway. I'll remove this.

colesbury · 2025-05-12T22:05:29Z

peps/pep-0788.rst

+On free-threaded builds, lock-ordering deadlocks are still possible
+if thread A acquired the lock for object A and then object B, and then
+another thread tried to acquire those locks in the reverse order. Free-threading
+currently protects against this by releasing locks when the thread state is
+detached, making detachment a necessity to prevent deadlocks.


I think this is mixing up a few ideas (critical sections and stop the world). You want Py_BEGIN_ALLOW_THREADS around blocking operations to prevent deadlock with GC (or other stop-the-world) events.

Stop-the-world events are analogous to holding the GIL -- one thread has exclusive access to the interpreter -- so you can end up with the same deadlocks.

Hm, is the current thing I wrote wrong? I think this paragraph is quite long already, and it's only here to quickly explain why Py_BEGIN_ALLOW_THREADS is still needed on free-threaded builds. If there's nothing wrong with what's already there, it's probably fine to leave it.

I agree that it's getting long already. Was this expanded in response to feedback on the PEP? I would just have one sentence regarding the free threaded build at the end of the lock ordering deadlock paragraph. Something like:

...while thread B holds the GIL and is waiting on the lock. A similar deadlock can occur in the free threaded build during stop the world pauses, which happen during during garbage collection.

Hm, is the current thing I wrote wrong?

The first part is definitely true: "on free-threaded builds, lock-ordering deadlocks are still possible".

The subsequent explanation doesn't make much sense to me. The "releasing locks when the thread state is
detached" behavior is only applicable to critical sections, and we don't use Py_BEGIN_ALLOW_THREADS directly with critical section APIs. The critical section API implementations do the detaching/reattaching internally.

Was this expanded in response to feedback on the PEP? I would just have one sentence regarding the free threaded build at the end of the lock ordering deadlock paragraph.

Ok, yeah, I'll do it this way.

The critical section API implementations do the detaching/reattaching internally.

I was thinking of something like this:

Py_BEGIN_CRITICAL_SECTION(whatever); acquire_os_lock(); // NOT PyMutex Py_END_CRITICAL_SECTION();

You still need to have Py_BEGIN_ALLOW_THREADS to release that critical section. Otherwise you can get lock-ordering deadlocks, right?

colesbury · 2025-05-12T22:09:40Z

peps/pep-0788.rst

-Daemon threads can cause finalization deadlocks
-***********************************************
+Daemon Threads Can Deadlock Finalization
+****************************************


The scenario here that leads to deadlock here happens when you acquire a lock during an object destructor/finalizer thats also acquired elsewhere. But if you're doing that, non-daemon threads won't save you because you will still occasionally deadlock with the GC.

Hmm, I wasn't aware of this. That sounds like it compromises some of the motivation behind the PEP. Could you go into a little more detail here?

Python's GC runs object finalizers (i.e., __del__ methods) in the same thread that triggered the GC, so you can end trying to reacquire a lock you already hold.

Here's an example from stackoverflow: https://stackoverflow.com/questions/18774401/self-deadlock-due-to-garbage-collector-in-single-threaded-code

Java runs finalizers in their own thread to avoid this sort of problem.

Generally, the "fix" in Python is to avoid acquiring locks in __del__ functions.

Ah, that makes sense, I wasn't taking the garbage collector into account. I was imagining some C code that acquired a lock and got hung before calling into Python. But I think you're right that PyThreadState_Ensure isn't sufficient to fix tp_finalize/__del__ deadlocks. That's not good.

Java runs finalizers in their own thread to avoid this sort of problem.

Thinking out loud--it's not out of the question to take a similar approach for Python, at least for non-daemon threads. I'm not too sure what gc.garbage does in modern versions, but in theory, we could put objects collected under a non-daemon thread state into a garbage list, and then finalize them all in a dedicated thread.

peps/pep-0788.rst

colesbury · 2025-05-12T22:37:03Z

peps/pep-0788.rst

+no closure, it's not possible for the caller to pass any objects or
+interpreter-specific data, so it's completely safe to choose the main


I don't see how "it's completely safe to choose the main interpreter" follows from "the callback has no closure".

Maybe these use cases (like readline) only make sense in the main interpreter, but choosing the correct interpreter seems important.

If you don't have a closure argument/callback parameter or some other workaround, then you can't pass interpreter-specific data, so you're good to assume that the main interpreter is the one you want. It might not be the right choice, but by "totally safe", I mean that things will just raise a completely safe exception (e.g. an AttributeError), rather than crashing through using an object from a different interpreter.

So basically, if you can access state from the caller, then you can also store the interpreter reference, and if not, then there's no way to access invalid cross-interpreter state anyway.

ZeroIntensity · 2025-05-13T01:51:51Z

colesbury · 2025-05-13T02:56:17Z

ZeroIntensity · 2025-05-13T13:09:09Z

vstinner · 2025-05-14T12:05:14Z

ZeroIntensity added 15 commits May 1, 2025 17:01

Clarify what 'native thread' means.

64c88b3

Add a section clarifying finalization and change up some wording.

bda3db1

Rewrite the abstract.

a57686c

A bunch of changes to the motivation and rationale.

3387f81

Add PyThreadState_GetDaemon() and reword the deprecation rationale.

ceeefea

Rewrite the entire damn specification.

3cbfb26

Update the rejected ideas.

d9de49a

Fix some outdated references.

c742d93

Fix typo in rejected ideas.

ad1bf7f

Adjust threading section.

bca6131

Specify that PyInterpreterRef is pointer-sized

868cdef

Add clarity to reference counting.

6b3a447

Fix typo in example.

f5e1af8

Formalize the headings.

98e7fcc

Add a terminology section.

95916a7

ZeroIntensity marked this pull request as ready for review May 4, 2025 17:55

ZeroIntensity requested a review from vstinner as a code owner May 4, 2025 17:55

ZeroIntensity added 4 commits May 4, 2025 15:03

Add PyInterpreterState_AsStrong()

257a252

Add an example for PyInterpreterState_AsStrong()

6b9b74e

An editorial pass.

48624ef

Fix typo in example.

31d3f75

ZeroIntensity requested a review from AA-Turner May 4, 2025 20:02

ZeroIntensity added 2 commits May 5, 2025 17:32

Some clarifications and a new example.

8440057

Fix wording.

9b08bf0

vstinner reviewed May 8, 2025

View reviewed changes

ZeroIntensity and others added 3 commits May 9, 2025 07:21

Update peps/pep-0788.rst

0e5acc8

Co-authored-by: Victor Stinner <vstinner@python.org>

Update peps/pep-0788.rst

6d96645

Co-authored-by: Victor Stinner <vstinner@python.org>

Apply suggestions from code review

8000

a229f7b

Co-authored-by: Victor Stinner <vstinner@python.org>

ZeroIntensity added 2 commits May 9, 2025 07:26

Merge branch 'pep-788-round-1' of https://github.com/ZeroIntensity/peps…

f8b0112

… into pep-788-round-1

Fix typos.

2332d3e

ZeroIntensity added 2 commits May 10, 2025 10:28

Use non-pointers for PyInterpreterRef

d5630af

Change the API for PyInterpreterState_AsStrong() and PyInterpreterWea…

86b4b79

…kRef_AsStrong()

vstinner approved these changes May 12, 2025

View reviewed changes

peps/pep-0788.rst Outdated Show resolved Hide resolved

Don't specify setting NULL

3212a61

colesbury reviewed May 12, 2025

View reviewed changes

ZeroIntensity added 3 commits May 12, 2025 20:38

infinitely -> unbounded

6e3550c

Reword 'extremely common'.

6f45d71

Use 'callback parameter' instead of 'closure'.

1d41eb6

ZeroIntensity requested a review from ericsnowcurrently May 18, 2025 18:03

ZeroIntensity added 3 commits May 18, 2025 15:22

Don't steal a reference in PyThreadState_Ensure().

2a75bfd

Remove the rest of reference theft.

1e6285f

Remove 'daemon'-ness as a property of threads.

bcc1c73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PEP 788: Address feedback from first discussion round #4400

Are you sure you want to change the base?

PEP 788: Address feedback from first discussion round #4400

PEP 788: Address feedback from first discussion round #4400

Are you sure you want to change the base?

PEP 788: Address feedback from first discussion round #4400

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment