All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

I am unable to reproduce the failure of test_queue, seen in https://github.com/python/cpython/actions/runs/14844414631/job/41674826787?pr=133466, even after running the test repeatedly with high levels of --parallel-threads. I will update the branch with a fix for the doc issue shortly.

TSan failure looks unrelated. I've restarted the job for you.

@ZeroIntensity wrote:

I agree with the motivation, but PyGILState_Ensure is evil.

Yikes. I knew it could be trouble but I didn't know it could be that bad. I'm still reading the PEP and will have more to say once I have finished.

We might want this in a new API (e.g., something like PyErr_CheckSignalsFast).

I would argue that relaxing a requirement is not a breaking change, but I'm willing to give the function a new name if you all think it is necessary.

So I think PEP 788 is trying to solve a related problem with thread states to the one I have in this PR, but it doesn't directly address my problem.

I set out to make PyErr_CheckSignals be a function that could be called whether or not the calling thread has an attached thread state. Perhaps this would be clearer with an example: 3e25a93 (the second of the two commits in this PR) makes this abstract change...

     int res;
+    Py_BEGIN_ALLOW_THREADS
     do {
-        Py_BEGIN_ALLOW_THREADS
         res = some_system_call(arguments...);
-        Py_END_ALLOW_THREADS
     } while (res < 0 && errno == EINTR && !PyErr_CheckSignals());
+    Py_END_ALLOW_THREADS

to some but not all of the loops with that structure in the CPython codebase.

PyGILState_Ensure() is the only existing C-API function (as far as I can tell) that you're allowed to call from either inside or outside a Py_BEGIN_ALLOW_THREADS ... Py_END_ALLOW_THREADS block, and it promises that when it returns you'll have an attached thread state. PyEval_RestoreThread (either directly or in the form of Py_BLOCK_THREADS) deadlocks in GIL builds if called by a thread that already holds the GIL, so that's no good; the lower-level _PyThreadState_Attach appears to have the same problem; PyThreadState_Swap does something unrelated.

But PyGILState_Ensure does considerably more than what I need. If I understood PEP 788 correctly, it's the bits I don't need -- particularly, creation of a temporary thread state for a thread that has never had one -- that are troublesome. PEP 788 sets out to fix those bits, but it doesn't add a new API that does the specific thing I need ("reattach this thread's state if it's detached") without any of the extra bits.

Tangentially, part of the problem is that PyErr_CheckSignals doesn't take an interpreter or thread-state argument, so I do need PyGILState_Ensure's machinery for guessing which thread state to reattach; this is perhaps an argument for leaving it alone and adding a PyErr_CheckSignalsDetached(tstate) function as suggested above. Connected to that is that Py_BEGIN_ALLOW_THREADS doesn't overtly give you the thread state handle it just detached; it's a hidden variable. I'm tempted to suggest soft-deprecating BEGIN/END_ALLOW_THREADS in favor of a new macro which would be used like this (for the same sample loop as above):

    int res;
    Py_WITH_DETACHED_TSTATE (ts) {
        do {
            res = some_system_call(arguments...);
         } while (res < 0 && errno == EINTR
                  && !PyErr_CheckSignalsDetached(ts));
    }

(A macro like this can be implemented in plain C by off-label use of a for loop.)

I'm open to helping with work toward this end, but I would very much like to find a way to do it independently of this PR, which I would like to keep focused on the goal described in #133465.

I think we don't have to worry about the cross-interpreter problems because subinterpreters can't handle signals anyway--that's a job for the main thread and main interpreter. My approach would be to check PyGILState_GetThisThreadState() == &_PyRuntime._main_interpreter._initial_thread. That shields us from any concurrent finalization shenanigans, so it should be a bit more thread-safe. If that passes, PyGILState_Ensure should be safe.

Also cc @encukou @vstinner as members of the C API WG. I'm not sure what the rules are for extending functionality of stable APIs. Should this be in a new function?

The main issue with this kind of extension is that code tested with new versions of Python will fail in older ones.
It can be extended, but I'd prefer adding a new function instead.

So I think we have consensus that this PR should introduce a new function rather than change the semantics of an existing one. I propose to call the new function PyErr_CheckSignalsDetached and I'll go ahead and start working on that change.

I see some related design issues that need to be resolved, though:

• Should the new function take a PyThreadState* argument?

  If it does, it's substantially easier to implement, but callers need to supply that argument, which can be difficult, both because it might need to be passed down through several layers of intermediate functions and because Py_BEGIN_ALLOW_THREADS does not officially give one access to a PyThreadState (the _state variable it declares is an undocumented implementation detail).

• Should one be allowed to call the new function with or without an attached thread state, or only without?

  Again this is a tradeoff between implementation and usage convenience. Implementation would be easier if we required users of the new function to not have an attached thread state. As far as I can tell, the only function with the semantics of "give me an attached thread state if I don't already have one, do nothing if I do" is PyGILState_Ensure, which, per PEP 788, we'd rather not introduce new uses of. PyEval_RestoreThread is documented to deadlock if called by a thread that already has an attached thread state; PyThreadState_Swap is documented to require the outgoing thread state to be attached.

  However, it would be substantially easier for extension module authors to use the new API if they didn't have to worry about whether they did or didn't hold an attached thread state at each point where they want to introduce a signals check/cancellation point.

• Should the new function run the cycle collector?

  PyErr_CheckSignals only actually checks for signals if it's running in the main interpreter on the main thread. However, regardless of which interpreter or thread it's running on, it may run the cycle collector (the "do we need to run the cycle collector soon?" flag is also checkable without the GIL/an attached thread state). This is not currently documented (this PR adds it to the documentation en passant).

  If the new function shouldn't try to run the cycle collector, it's easier to implement; in particular, the question of how we access the correct thread state becomes simpler. However, the rationale for PyErr_CheckSignals having the side effect of running the cycle collector is

  Opportunistically check if the GC is scheduled to run and run it if we have a request. This is done here because native code needs to call this API if is going to run for some time without executing Python code to ensure signals are handled. Checking for the GC here allows long running native code to clean cycles created using the C-API even if it doesn't run the evaluation loop

  which would seem to apply equally to the new PyErr_CheckSignalsDetached.

• While we're in here, do we need is_tripped anymore?

  The actual C signal handler sets three semi-redundant flags: the per-signal &Handlers[sig_num].tripped, the global variable is_tripped, and the _PY_SIGNALS_PENDING_BIT bit in the eval breaker word for _PyRuntime.main_tstate. As far as I can tell, is_tripped is fully redundant to the eval breaker bit. Is there anything that needs it that isn't apparent to a recursive grep of the CPython source tree?

Let's open an issue with the C API working group so they can bicker about bikeshedding and the API.

In person at the sprints, Guido suggested that this PR should be split up into three: one each for the core functional changes, the docs changes, and the "look what we can do now" changes to various stdlib modules. I'll make that happen, but not until Wednesday. Meantime, can we please focus discussion on the list of questions I posted? @ZeroIntensity I'd appreciate it if you could open that issue; I'm tired enough that I'm making foolish mistakes.

@zackw: Sorry we didn't get a chance to say goodbye at the sprint.

Let me try answering your questions, briefly.

* [ ]  Should the new function take a `PyThreadState*` argument?

I agree with your current choice to not ask for a thread state -- we want the function to be as convenient as possible, and to add minimum friction.

* [ ]  Should one be allowed to call the new function _with or without_ an attached thread state, or only _without_?

I think only without. I can imagine many patterns that make it easier for extensions to choose between PyErr_CheckSignals and PyErr_CheckSignals_Detached.

* [ ]  Should the new function run the cycle collector?

I think not. Calling the cycle collector is important when holding the GIL because we might be creating/updating/destroying Python objects. But when not holding the GIL we shouldn't be doing that, so the set of objects shouldn't change at all! Other threads that are creating lots of objects from C should be calling PyErr_CheckSignals(), so they'll run the GC (unlike signal handlers, it's not limited to the main thread).

* [ ]  While we're in here, do we need `is_tripped` anymore?
  The actual C signal handler sets _three_ semi-redundant flags: the per-signal `&Handlers[sig_num].tripped`, the global variable `is_tripped`, and the `_PY_SIGNALS_PENDING_BIT` bit in the eval breaker word for `_PyRuntime.main_tstate`.  As far as I can tell, `is_tripped` is fully redundant to the eval breaker bit.  Is there anything that needs it that isn't apparent to a recursive grep of the CPython source tree?

I don't know, but it sounds like a "here be dragons" area to me. Let's not try to fix this and then found out we shouldn't have. Someone else (or you :-) could endeavor to remove is_tripped at a later point.

It does look like you need to do a manual merge of a newer main branch before we merge this, since the branch is flagged as having conflicts. I recommend doing that at the latest possible moment (e.g. when you've received all the Approvals you need) so you won't have to do twice. Maybe moving the docs and other files to new PRs will remove the conflict.

Should the new function take a PyThreadState* argument? Py_BEGIN_ALLOW_THREADS does not officially give one access to a PyThreadState (the _state variable it declares is an undocumented implementation detail).

I'll dissent here. It should, just like Py_END_ALLOW_THREADS explicitly grabs the locally saved state.

A new macro like Py_CHECK_SIGNALS would both keep the secret variable hidden and make this easy to use.

(For the record, it's documented, the question being is whether that's a guarantee of how things work or just an illustration. In my opinion, it is at least an implementation guide for non-C/C++ users.)

Should the new function take a PyThreadState* argument? Py_BEGIN_ALLOW_THREADS does not officially give one access to a PyThreadState (the _state variable it declares is an undocumented implementation detail).

I'll dissent here. It should, just like Py_END_ALLOW_THREADS explicitly grabs the locally saved state.

A new macro like Py_CHECK_SIGNALS would both keep the secret variable hidden and make this easy to use.

(For the record, it's documented, the question being is whether that's a guarantee of how things work or just an illustration. In my opinion, it is at least an implementation guide for non-C/C++ users.)

I don't understand why those save/restore operations need to be done using a macro. They could just as easily be done by a function that does that save/restore behavior itself. I don't want to turn the old PyErr_CheckSignals (unchanged in functionality, just refactored) into a macro, and I don't believe things are so perf-critical that the first checks in PyErr_CheckSignals_Detached need to be a macro.

So I still don't think there's anything wrong with calling a function that retrieves and restores the tstate.

Well, explicit is better than implicit.

The code between Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS is usually low-level, but that's not a requirement. It can do anything. Including attaching thread states to do Python work, or even making (sub)interpreters come and go. How sure can we be that the this state can't be clobbered?

For example:

#include <Python.h>
#include <stdio.h>

#define N_ITERATIONS 2
static void call_into_some_library(void);

// For simplicity, store our main interpreter globally
PyInterpreterState *main_interp = NULL;

static PyObject *
demo_func(PyObject *mod, PyObject *arg) {
    main_interp = PyInterpreterState_Get();

    printf("Start! This thread state = %p\n",
           PyGILState_GetThisThreadState());

    Py_BEGIN_ALLOW_THREADS;
    for (int i=0; i<N_ITERATIONS; i++) {
        // Do some C work using some library...
        call_into_some_library();
        // if PyGILState_GetThisThreadState() is NULL, the
        // PyErr_CheckSignals from this PR will crash
        printf("%d/%d: this thread state = %p\n", i, N_ITERATIONS,
               PyGILState_GetThisThreadState());
    }
    Py_END_ALLOW_THREADS;
    Py_RETURN_NONE;
}

// Meanwhile, the library wants to do some Python...

static void
call_into_some_library(void) {
    /* ... */
    if (PyThreadState_GetUnchecked() == NULL) {
        // no thread state attached!
        // so make one & attach it
        PyThreadState *ts = PyThreadState_New(main_interp);
        PyEval_AcquireThread(ts);
        // call some fun Python API
        PyObject *s = PyUnicode_FromString("all OK down here\n");
        PyObject_Print(s, stdout, Py_PRINT_RAW);
        Py_DECREF(s);
        // and get rid of the thread state
        PyThreadState_Clear(ts);
        PyThreadState_DeleteCurrent();
        // by this we have reset "this" thread state to NULL
    }
    else { /* ... */ }
}

static PyModuleDef module = {
    .m_name = "extension",
    .m_methods = (PyMethodDef[]) {
        {"demo_func", demo_func, METH_NOARGS},
        {0},
    },
};

PyMODINIT_FUNC
PyInit_extension(void) {
    return PyModuleDef_Init(&module);
}

I don't understand why those save/restore operations need to be done using a macro. They could just as easily be done by a function that does that save/restore behavior itself.

I agree with that part, the macros could just have been functions. But they should be explicit about the state they want to get back to.

The new function is implemented with PyGILState_Ensure() and PyGILState_Release() which doesn't support sub-interpreters. I'm not sure if it's a good idea to add such new function. We may check the wrong interpreter. The current PyErr_CheckSignals() API doesn't have such issue.

    PyGILState_STATE st = PyGILState_Ensure();
    int err = check_signals_attached(tstate, cycle_collect);
    PyGILState_Release(st);

It might be fine, because subinterpreters don't currently support handling signals anyway. But, I don't know if that will always be the case. I was talking to Eric at the sprints about handling signals in any interpreter active in the main thread.

It might be fine, because subinterpreters don't currently support handling signals anyway.

Let's say that the function is called in a subinterpreter running in the main thread, and PyGILState_Ensure() picks instead the main thread of the main interpreter: we can handle signals in this case, whereas subinterpreters doesn't support handling signals. There is a problem, no?

Hmm, yeah, that doesn't sound great.

Yeah, I think we need a somewhat different design to handle subinterpreters and free threading smoothly.

And PyErr_CheckSignals may need to be changed as well? Maybe the forward-thinking approach would be a single function then??? (Sorry @zackw).

int PyErr_CheckSignals_Detached(PyInterpreterState *interp) is probably good enough. I don't think we'll need anything different for free-threading.

I'm not convinced that we have to provide a way to "Allow PyErr_CheckSignals to be called without holding the GIL". PEP 788 shows that it's quite complicated to acquire/release the GIL, there are corner cases and we need a new (more elaborated) API to prevent bugs/crashes.

int PyErr_CheckSignals_Detached(PyInterpreterState *interp)

Is there a risk that the interpreter pointer can become a dangling pointer?

Is there a risk that the interpreter pointer can become a dangling pointer?

Yes, but that's true of the existing APIs too. If PEP 788 is accepted, we can change this to use a strong interpreter reference.

The function already checks that it is the main thread. Is there a way to check that we’re in the main interpreter? Assuming we can check those without having the GIL or a tstate, would that simplify the requirements? Or is there still ambiguity?

I think the goal is to eventually get signal handling working in subinterpreters that are running in the main thread too.

Obligatory ping @ericsnowcurrently

Yeah, that's a possibility. We'll have to see what makes the most sense.

I think the goal is to eventually get signal handling working in subinterpreters that are running in the main thread too.

Let's let the folks working on that feature worry about it.

But is there way to check that you're in the main interpreter?

_Py_IsMainInterpreter