Skip to content

gh-124622: Add PyGILState_EnsureOrFail() function #129688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Doc/c-api/init.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1279,6 +1279,21 @@ with sub-interpreters:
Hangs the current thread, rather than terminating it, if called while the
interpreter is finalizing.


.. c:function:: int PyGILState_EnsureOrFail(PyGILState_STATE *state)

Similar to :c:func:`PyGILState_Ensure`, except that it returns with a status
code even in the case of failure. Specifically, it returns ``0`` when the
operation succeeded, and ``-1`` otherwise. In contrast to
:c:func:`PyGILState_Ensure`, *state* is an argument.
Comment on lines +1287 to +1288
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
operation succeeded, and ``-1`` otherwise. In contrast to
:c:func:`PyGILState_Ensure`, *state* is an argument.
operation succeeded, and ``-1`` otherwise.
The return value of :c:func:`!PyGILState_Ensure`
is stored in *\*state*.


In the case of failure, it is *unsafe* to use the Python API following the
call. Releasing the obtained *state* via :c:func:`PyGILState_Release` should
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
call. Releasing the obtained *state* via :c:func:`PyGILState_Release` should
call. Releasing the obtained *state* via :c:func:`PyGILState_Release` must

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied this suggestion to #130012 since I closed this PR.

only be done in the case of success.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the mention that these are “not compatible with sub-interpreters” directly to the docs?
Which API should be recommended instead, if this one is not compatible with sub-interpreters?

I meant repeating the note above PyGILState_Ensure that says The following functions use thread-local storage, and are not compatible with sub-interpreters:. Users that click the What's New link go directly to the function docs and are quite likely to miss the warning.
Something like:

Suggested change
Like :c:func:`!PyGILState_Ensure`, this function is not safe to use with
supinterpreters, since it is not possible to specify which interpreter
is activated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#130012 PR should be subinterpreter-friendly, I closed this PR instead.

.. versionadded:: next


.. c:function:: void PyGILState_Release(PyGILState_STATE)

Release any resources previously acquired. After this call, Python's state will
Expand Down
1 change: 1 addition & 0 deletions Doc/data/stable_abi.dat

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Doc/whatsnew/3.14.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1334,6 +1334,9 @@ New features
and get an attribute of the module.
(Contributed by Victor Stinner in :gh:`128911`.)

* Add :c:func:`PyGILState_EnsureOrFail` function: similar to
:c:func:`PyGILState_Ensure`, but return ``-1`` if the thread must exit.
(Contributed by Victor Stinner in :gh:`124622`.)

Limited C API changes
---------------------
Expand Down
2 changes: 2 additions & 0 deletions Include/internal/pycore_ceval.h
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,8 @@ void _Py_unset_eval_breaker_bit_all(PyInterpreterState *interp, uintptr_t bit);

PyAPI_FUNC(PyObject *) _PyFloat_FromDouble_ConsumeInputs(_PyStackRef left, _PyStackRef right, double value);

extern int _PyEval_AcquireLockOrFail(PyThreadState *tstate);
extern int _PyEval_RestoreThreadOrFail(PyThreadState *tstate);

#ifdef __cplusplus
}
Expand Down
2 changes: 2 additions & 0 deletions Include/internal/pycore_pystate.h
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@ _Py_AssertHoldsTstateFunc(const char *func)
#define _Py_AssertHoldsTstate()
#endif

extern int _PyThreadState_AttachOrFail(PyThreadState *tstate);

#ifdef __cplusplus
}
#endif
Expand Down
5 changes: 5 additions & 0 deletions Include/pystate.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ PyAPI_FUNC(PyGILState_STATE) PyGILState_Ensure(void);
*/
PyAPI_FUNC(void) PyGILState_Release(PyGILState_STATE);

#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030e0000
/* New in 3.14 */
PyAPI_FUNC(int) PyGILState_EnsureOrFail(PyGILState_STATE *state);
#endif

/* Helper/diagnostic function - get the current thread state for
this thread. May return NULL if no GILState API has been used
on the current thread. Note that the main thread always has such a
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_stable_abi_ctypes.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Add :c:func:`PyGILState_EnsureOrFail` function: similar to
:c:func:`PyGILState_Ensure`, but return ``-1`` if the thread must exit.
Patch by Victor Stinner.
2 changes: 2 additions & 0 deletions Misc/stable_abi.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2545,3 +2545,5 @@
added = '3.14'
[function.Py_PACK_VERSION]
added = '3.14'
[function.PyGILState_EnsureOrFail]
added = '3.14'
4 changes: 3 additions & 1 deletion Modules/_testcapimodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1275,7 +1275,9 @@ temporary_c_thread(void *data)
PyThread_release_lock(test_c_thread->start_event);

/* Allocate a Python thread state for this thread */
state = PyGILState_Ensure();
if (PyGILState_EnsureOrFail(&state) < 0) {
abort();
}

res = PyObject_CallNoArgs(test_c_thread->callback);
Py_CLEAR(test_c_thread->callback);
Expand Down
1 change: 1 addition & 0 deletions PC/python3dll.c

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

64 changes: 51 additions & 13 deletions Python/ceval_gil.c
Original file line number Diff line number Diff line change
Expand Up @@ -277,12 +277,15 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int final_release)

/* Take the GIL.

Return 0 on success.
Return -1 if the thread must exit.

The function saves errno at entry and restores its value at exit.
It may hang rather than return if the interpreter has been finalized.

tstate must be non-NULL. */
static void
take_gil(PyThreadState *tstate)
static int
take_gil_or_fail(PyThreadState *tstate)
{
int err = errno;

Expand All @@ -304,15 +307,15 @@ take_gil(PyThreadState *tstate)
C++. gh-87135: The best that can be done is to hang the thread as
the public APIs calling this have no error reporting mechanism (!).
*/
PyThread_hang_thread();
goto tstate_must_exit;
}

assert(_PyThreadState_CheckConsistency(tstate));
PyInterpreterState *interp = tstate->interp;
struct _gil_runtime_state *gil = interp->ceval.gil;
#ifdef Py_GIL_DISABLED
if (!_Py_atomic_load_int_relaxed(&gil->enabled)) {
return;
goto done;
}
#endif

Expand Down Expand Up @@ -348,9 +351,7 @@ take_gil(PyThreadState *tstate)
if (drop_requested) {
_Py_unset_eval_breaker_bit(holder_tstate, _PY_GIL_DROP_REQUEST_BIT);
}
// gh-87135: hang the thread as *thread_exit() is not a safe
// API. It lacks stack unwind and local variable destruction.
PyThread_hang_thread();
goto tstate_must_exit;
}
assert(_PyThreadState_CheckConsistency(tstate));

Expand All @@ -366,7 +367,7 @@ take_gil(PyThreadState *tstate)
// return.
COND_SIGNAL(gil->cond);
MUTEX_UNLOCK(gil->mutex);
return;
goto done;
}
#endif

Expand Down Expand Up @@ -401,7 +402,7 @@ take_gil(PyThreadState *tstate)
/* tstate could be a dangling pointer, so don't pass it to
drop_gil(). */
drop_gil(interp, NULL, 1);
PyThread_hang_thread();
goto tstate_must_exit;
}
assert(_PyThreadState_CheckConsistency(tstate));

Expand All @@ -411,8 +412,25 @@ take_gil(PyThreadState *tstate)

MUTEX_UNLOCK(gil->mutex);

#ifdef Py_GIL_DISABLED
done:
#endif
errno = err;
return;
return 0;

tstate_must_exit:
errno = err;
return -1;
}

static void
take_gil(PyThreadState *tstate)
{
if (take_gil_or_fail(tstate) < 0) {
// gh-87135: hang the thread as *thread_exit() is not a safe
// API. It lacks stack unwind and local variable destruction.
PyThread_hang_thread();
}
}

void _PyEval_SetSwitchInterval(unsigned long microseconds)
Expand Down Expand Up @@ -586,6 +604,13 @@ _PyEval_AcquireLock(PyThreadState *tstate)
take_gil(tstate);
}

int
_PyEval_AcquireLockOrFail(PyThreadState *tstate)
{
_Py_EnsureTstateNotNULL(tstate);
return take_gil_or_fail(tstate);
}

void
_PyEval_ReleaseLock(PyInterpreterState *interp,
PyThreadState *tstate,
Expand Down Expand Up @@ -641,19 +666,32 @@ PyEval_SaveThread(void)
return tstate;
}

void
PyEval_RestoreThread(PyThreadState *tstate)

int
_PyEval_RestoreThreadOrFail(PyThreadState *tstate)
{
#ifdef MS_WINDOWS
int err = GetLastError();
#endif

_Py_EnsureTstateNotNULL(tstate);
_PyThreadState_Attach(tstate);
if (_PyThreadState_AttachOrFail(tstate) < 0) {
return -1;
}

#ifdef MS_WINDOWS
SetLastError(err);
#endif
return 0;
}


void
PyEval_RestoreThread(PyThreadState *tstate)
{
if (_PyEval_RestoreThreadOrFail(tstate) < 0) {
PyThread_hang_thread();
}
}


Expand Down
42 changes: 35 additions & 7 deletions Python/pystate.c
Original file line number Diff line number Diff line change
Expand Up @@ -2060,8 +2060,8 @@ tstate_wait_attach(PyThreadState *tstate)
} while (!tstate_try_attach(tstate));
}

void
_PyThreadState_Attach(PyThreadState *tstate)
int
_PyThreadState_AttachOrFail(PyThreadState *tstate)
{
#if defined(Py_DEBUG)
// This is called from PyEval_RestoreThread(). Similar
Expand All @@ -2076,7 +2076,9 @@ _PyThreadState_Attach(PyThreadState *tstate)


while (1) {
_PyEval_AcquireLock(tstate);
if (_PyEval_AcquireLockOrFail(tstate) < 0) {
return -1;
}

// XXX assert(tstate_is_alive(tstate));
current_fast_set(&_PyRuntime, tstate);
Expand Down Expand Up @@ -2111,6 +2113,15 @@ _PyThreadState_Attach(PyThreadState *tstate)
#if defined(Py_DEBUG)
errno = err;
#endif
return 0;
}

void
_PyThreadState_Attach(PyThreadState *tstate)
{
if (_PyThreadState_AttachOrFail(tstate) < 0) {
PyThread_hang_thread();
}
}

static void
Expand Down Expand Up @@ -2730,8 +2741,9 @@ PyGILState_Check(void)
return (tstate == tcur);
}

PyGILState_STATE
PyGILState_Ensure(void)

int
PyGILState_EnsureOrFail(PyGILState_STATE *state)
{
_PyRuntimeState *runtime = &_PyRuntime;

Expand Down Expand Up @@ -2770,7 +2782,10 @@ PyGILState_Ensure(void)
}

if (!has_gil) {
PyEval_RestoreThread(tcur);
if (_PyEval_RestoreThreadOrFail(tcur) < 0) {
*state = PyGILState_UNLOCKED;
return -1;
}
}

/* Update our counter in the thread-state - no need for locks:
Expand All @@ -2780,9 +2795,22 @@ PyGILState_Ensure(void)
*/
++tcur->gilstate_counter;

return has_gil ? PyGILState_LOCKED : PyGILState_UNLOCKED;
*state = has_gil ? PyGILState_LOCKED : PyGILState_UNLOCKED;
return 0;
}


PyGILState_STATE
PyGILState_Ensure(void)
{
PyGILState_STATE state;
if (PyGILState_EnsureOrFail(&state) < 0) {
PyThread_hang_thread();
}
return state;
}


void
PyGILState_Release(PyGILState_STATE oldstate)
{
Expand Down
Loading