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

The Word "Finalizing" in C-API Function Names is Sometimes Misleading #110490

Open
ericsnowcurrently opened this issue Oct 6, 2023 · 3 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API

Comments

@ericsnowcurrently
Copy link
Member

(This is something I've been thinking about in the last few weeks and has before more significant as we've added Py_IsFinalizing() to the 3.13 public C-API1.)

tl;dr I think that either we should make the Py_IsFinalizing() behavior match the name more closely or we should change the name. Internally we should think in terms of what runtime capabilities are available rather than just "is finalizing".


As of recently, we have Py_IsFinalizing() in the public C-API, along with various similarly named (.*[Ff]inalizing.*) functions and runtime state fields in the internal API. 2 In addition, sys.is_finalizing() has been around since 3.53.

They all serve to track/inform when the current runtime/interpreter is shutting down and therefore might not be in a stable state (leading to crashes). The intention is clear in the docs for Py_IsFinalizing():

int Py_IsFinalizing()
    Return true (non-zero) if the main Python interpreter is shutting down. Return false (zero) otherwise.

Internally, we use that status to determine the availability of certain capabilities, most notably threading. In fact, the whole family of functions and state started in 2011 as part of the effort to deal with daemon threads during shutdown.

This has lead to a bit of a conflict in the meaning of "finalizing". IMHO, it would be worth sorting out the discrepancy. I expect this will include either changing the behavior of Py_IsFinalizing() or changing the name of the function to match what it is actually reporting.

(Is this a critical issue? No. The matter at hand is fundamentally related to daemon threads. 🤮)
(Is it worth thinking through anyway? Yes. I expect the discussion would help bring more clarity to runtime finalization and to the runtime in general.)

Context

Finalization is started for the runtime by Py_Finalize() and Py_FinalizeEx(). For an interpreter we use Py_EndInterpreter(). (For thread states we don't have a specific API, but the closest is PyThreadState_Clear() + PyThreadState_Delete().)

Once finalization has begun, we keep track of that fact in a number of places:
  • _PyRuntimeState._finalizing
  • _PyRuntimeState._finalizing_id
  • PyInterpreterState.finalizing
  • PyInterpreterState._finalizing
  • PyInterpreterState._finalizing_id
  • PyThreadState._status.finalizing
We report it through a various API:
  • Py_IsFinalizing() [3.13] (docs: "Return true (non-zero) if the main Python interpreter is shutting down. Return false (zero) otherwise.")
  • sys.is_finalizing() [3.5] (docs: "Return True if the Python interpreter is shutting down, False otherwise.")
  • _PyRuntimeState_GetFinalizing()
  • _PyRuntimeState_GetFinalizingID()
  • _PyInterpreterState_GetFinalizing()
  • _PyInterpreterState_GetFinalizingID()
Here's a timeline:
  • [2008, ~3.0] bpo-1856 (AKA gh-46164) opened about crashes with daemon threads during runtime finalization
  • [2011, 3.2] added _Py_Finalizing as part of the effort to solve bpo-1856
  • [3.5] added sys.is_finalizing(), a light wrapper around _Py_Finalizing
  • [3.7] replaced _Py_Finalizing with _PyRuntimeState.finalizing; added _Py_IsFinalizing() (in "public" C-API)
  • [2019] (gh-80608) _Py_Finalizing() recommended for use in docs for PyEval_RestoreThread()
  • [2020, 3.9] added _Py_GetFinalizing(); renamed _PyRuntimeState.finalizing to _PyRuntimeState._finalizing (and made it atomic)
  • [July 2023, 3.13] (gh-106400) moved _Py_IsFinalizing() to the internal C-API
  • [Aug. 2023, 3.13] (gh-108014/gh-108032) added _Py_IsFinalizing() back to the public C-API as Py_IsFinalizing()
  • [this week] (gh-110397/gh-110441) discussion about adding Py_IsFinalizing() to the stable ABI

The Problem

The name Py_IsFinalizing (or sys.is_finalizing2) implies that the function tells you if the runtime is shutting down and will soon be unavailable. In fact, the docs actually say this. However, that explanation is only mostly accurate. When the function returns true, that is completely correct, but the same is not completely correct when it returns false. So, where might returning false be incorrect?

Currently, there is actually a meaningful space of time between when Py_Finalize() is called and when Py_IsFinalizing() returns true. In that time we do a number of things, like wait for all non-daemon threads to stop and run all global atexit hooks. Essentially, Py_IsFinalizing() returns true only at the point in finalization where other threads (i.e. not the current/main thread) should no longer rely on a stable runtime or C-API. (Perhaps the function should be called something like Py_AreThreadsAllowed() instead.)

Tracking that point in time is important for how we handle daemon threads during shutdown. We shouldn't change that.

To resolve any confusion and ambiguity here, we should instead:

  • explicitly acknowledge what we're actually tracking (in name and description)
  • decide what we care about internally (and why)
  • determine what information users actually want

Internally we care about two things: whether or not the runtime currently supports multithreading and whether or not the runtime (or current interpreter) is reaching the end of its life.

For users of PyEval_RestoreThread(), they want to know if the current thread will be terminated if they call that function to re-acquire the GIL. That matters for extensions running in daemon threads, and probably for some embedders. I'm not sure they care about whether or not Python is finalizing specifically.

What about other users? I don't know why they might want to know if the runtime is finalizing. It would certainly only be of interest for daemon threads (and for embedded applications).

What To Do About It?

It makes sense to figure out what folks actually care about when it comes to the concept of "finalizing". It would likewise make sense to ensure names and descriptions actually match what functions do (and state is for).

In the specific case of Py_IsFinalizing(), I see a couple options:

  1. change it to track the moment Py_Finalize() starts, before it actually does anything, so "finalizing" is 100% accurate
  2. change the name to match what it actually tracks: whether or not other threads can count on a stable runtime

(Likewise for sys.is_finalizing(), and most internal API.)

Here are some tricky things to consider:

  • we want to disable some capabilities as soon as possible during shutdown (e.g. disallow creating new threads before waiting for existing non-daemon threads to finish)
  • other threads are welcome to keep using the full runtime, even after shutdown begins, as long as we haven't started cleaning up state (which would then cause crashes) 4
  • in the main thread (executing Py_Finalize()), extension modules can (for now?) still run the risk of accessing invalid state after the point Py_IsFinalizing() returns true, up to the point that the extension module is cleaned up (see finalize_modules() in pylifecycle.c) and perhaps even after that

Footnotes

  1. ...and possibly to the stable ABI. See gh-110397: Add Py_IsFinalizing() to the stable ABI #110441.

  2. We also have the fundamentally related Py_Finalize() and Py_FinalizeEx(), which have been with us for a long, long time. 2

  3. sys.finalizing() is a thin wrapper around Py_IsFinalizing(). Before 3.13, it wrapped _Py_IsFinalizing(). Before 3.7, it wrapped _Py_Finalizing.

  4. This is the fundamental basis of the original "finalizing" status we introduced back in 2011.
    The one tricky thing is that we don't want

@vstinner
Copy link
Member

vstinner commented Oct 7, 2023

I merged my PR #110441 which adds Py_IsFinalizing() function to the stable ABI, since I'm feeling responsible of breaking applications relying on Python 3.12 private _Py_IsFinalizing() API. This C API exposes Python sys.is_finalizing() API which exists since Python 3.5.

For sure, there is always room for enhancement. It's a very complex topic, I wrote articles about it and I'm keeping notes about it: Python Finalization.

While it's possible to enhance the API, make finalization more reliable, reduce the risk of crashes, IMO we need "something" for people who are already affected by known issues. @wjakob elaborated issues that this API is solving in #110397 (comment) and just for that, I think that such API is worth it.

Python 3.13 didn't get an alpha1 release. We still have time until October 2024 to refine the documentation, the function name, come with a better API, etc. There is a non-zero risk that we will keep the name and so we will be good :-)

@vstinner
Copy link
Member

vstinner commented Oct 7, 2023

I'm not sure about the whole concept of "check if executing an issue will be safe" and then "execute the action". This pattern is known as https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use and is likely to cause race conditions. Here we are talking about threads which are running in parallel, "at the same time". So race conditions are likely, and it's not a theorical issue: I hit this exact problem in issue gh-110052 :-(

While the API involved in gh-110052 crash is different, the private _PyThreadState_MustExit(tstate) API, the problem is very similar :-(

Here, maybe the GIL is enough to keep the code safe. But hum, PEP 703 – Making the Global Interpreter Lock Optional in CPython is going to remove the GIL. Ooops.

Recently, a similar problem was introduced: _thread.start_new_thread() now raises RuntimeError during Python finalization. Problem: calling sys.is_finalizing() before calling _thread.start_new_thread() leads to https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use race condition. I fixed multiprocessing to handle this new RuntimeError: not by calling sys.is_finalizing(), but by catching RuntimeError. See my commit 6351842. Sadly, I made a mistake in the commit title: instead of PythonFinalizationError, you should read RuntimeError. While working on this fix, I considered to add a new PythonFinalizationError exception: see PR gh-109809. But I changed my mind since it was not strictly required to fix the issue. Maybe adding PythonFinalizationError would make the error message more explicit, and help users to understand what's going on.

In short, maybe we should redesign some APIs to make them safe to call during Python finalization: not crash, but return an error.

@vstinner
Copy link
Member

vstinner commented Oct 7, 2023

IMO sys.is_finalizing() and Py_IsFinalizing() remain useful. If they are true, you know for sure that some actions like spawning a thread will fail.

For example, sqlite3.Connection finalizer behaves differently if _Py_IsInterpreterFinalizing(interp) is true: it no longer removes callbacks and it clears exceptions rather than loggging exceptions with sys.excepthook. I don't know if it's the good behavior or not, but apparently, some people noticed issues during Python finalization, and these APIs give them a way to develop a workaround.

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
Projects
None yet
Development

No branches or pull requests

4 participants