Skip to content
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

Fail PyGILState_Ensure() If Finalizing? #124622

Open
ericsnowcurrently opened this issue Sep 26, 2024 · 9 comments
Open

Fail PyGILState_Ensure() If Finalizing? #124622

ericsnowcurrently opened this issue Sep 26, 2024 · 9 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API type-feature A feature request or enhancement

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Sep 26, 2024

Feature or enhancement

Proposal:

Once the runtime or current interpreter starts finalizing, we currently exit or stall the current thread as soon as it tries to acquire the GIL, via PyThread_exit_thread() (or PyThread_hang_thread() after gh-105805). That includes in PyGILState_Ensure(). However, in the case of the latter it might make more sense (i.e. friendlier) for the call to fail instead of exiting/hanging the current thread.

FWIW, this is a hypothetical situation I noticed while looking at pystate.c. Also, the same scenario would be affected by gh-124619.

CC @gpshead

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

@ericsnowcurrently ericsnowcurrently added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API 3.14 new features, bugs and security fixes labels Sep 26, 2024
@gpshead
Copy link
Member

gpshead commented Sep 26, 2024

A challenge in doing this is that PyGILState_Ensure returns an enum PyGILState_STATE value (defined in Include/pystate.h). That only contains LOCKED and UNLOCKED states today, so callers of the function in existing compiled Python C API code will never expect other values. We'd need to know if they were compiled against a recent enough Python version in order to return a new enum value representing an error because the interpreter is being finalized thus no GIL acquisition could possibly succeed.

@gpshead
Copy link
Member

gpshead commented Sep 26, 2024

I think a new GIL acquisition and release C API would be needed. The way the existing ones get used in existing C code is not amenible to suddenly bolting an error state onto; none of the existing C code is written that way. After the call they always just assume they have the GIL and can proceed. The API was designed as "it'll block and only return once it has the GIL" without any other option.

@ZeroIntensity
Copy link
Member

This is something that seems fatal-error worthy rather than a new PyGILState_STATE value -- there's nothing much a dev can do if they want to call the C API during a finalizing interpreter.

I think a new GIL acquisition and release C API would be needed.

That's been loosely discussed recently (in the context of subinterpreters, rather than the GIL), see #123728 (comment)

@rruuaanng
Copy link
Contributor

If you don't mind, I will sort out the requirements under discussion and publish some issue in the near future.

@picnixz picnixz removed the 3.14 new features, bugs and security fixes label Dec 15, 2024
@gpshead
Copy link
Member

gpshead commented Feb 2, 2025

Tying threads together, a new PyGILState_EnsureOrSafelyFail() is suggested for this issue in https://discuss.python.org/t/safely-using-the-c-api-when-python-might-shut-down/78850 as allowing modern extension code to actually be able to detect failure would be a good thing.

@ZeroIntensity
Copy link
Member

I don't think it's a good idea to add new things with the term "GIL" in them. I'd prefer a totally new API design (e.g., PyThreadState_Ensure) that takes an explicit interpreter rather than an implicit one.

@ZeroIntensity
Copy link
Member

Ok, I've been playing around with implementing this. I think we need to very clearly define two things:

  • Where should Python stop allowing thread states to be created? When the interpreter starts finalizing? When the runtime starts finalizing? But, there does seem to be some desire in the discussion to use the C API in finalizers, so maybe somewhere later?
  • If we go with the latter, then what should happen to threads that are created during finalization? Currently, Py_Finalize assumes that there's only one thread left, so there's no mechanism in place for getting rid of new thread states. (I do have an open PR that changes this for subinterpreters, but not the main interpreter yet.)

vstinner added a commit to vstinner/cpython that referenced this issue Feb 5, 2025
Add new "OrFail" functions:

* _PyEval_AcquireLockOrFail()
* _PyEval_RestoreThreadOrFail()
* _PyThreadState_AttachOrFail()
* take_gil_or_fail()
vstinner added a commit to vstinner/cpython that referenced this issue Feb 5, 2025
Add new "OrFail" functions:

* _PyEval_AcquireLockOrFail()
* _PyEval_RestoreThreadOrFail()
* _PyThreadState_AttachOrFail()
* take_gil_or_fail()
@vstinner
Copy link
Member

vstinner commented Feb 5, 2025

I created #129688 to add PyGILState_EnsureOrFail() function.

vstinner added a commit to vstinner/cpython that referenced this issue Feb 11, 2025
PyThreadState_Ensure() and PyThreadState_Release() functions.

Add new "OrFail" internal functions:

* _PyEval_AcquireLockOrFail()
* _PyEval_RestoreThreadOrFail()
* _PyThreadState_AttachOrFail()
* take_gil_or_fail()
vstinner added a commit to vstinner/cpython that referenced this issue Feb 11, 2025
Add PyThreadState_Ensure() and PyThreadState_Release() functions.

Add new "OrFail" internal functions:

* _PyEval_AcquireLockOrFail()
* _PyEval_RestoreThreadOrFail()
* _PyThreadState_AttachOrFail()
* take_gil_or_fail()
@vstinner
Copy link
Member

See also comments on a previous similar attempt: #123728

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

6 participants