The traversal function must not have any side effects. It must not modify the reference counts of any Python objects nor create or destroy any Python objects.
This primarily means you can’t use functions that return new references, or ones that raise (new) exceptions. (Basically, the kind of functions we’re trying to add nowadays.)
But you also can’t use any C API function, unless it guarantees that it has no side effects. I don’t think we guarantee that for anything.
You typically don’t need to do much in tp_traverse. But, both current PEPs for a free-threading stable ABI (PEP 803 & PEP 809) require some newer APIs for module state and PEP 697 layouts (instance structs that don’t include PyObject).
So, I’d like to add variants for existing functions that are usable in this context – they’d:
never set an exception (on failure, only return NULL)
return borrowed references
do not have “internal” side effects (for example, call a “normal C API function and then DECREF the result)
For the first two I’d normally name the functions with _NoError and _Borrow suffixes, but, perhaps that’s included in “safe to use in GC callbacks”. Would _GCSafe work?
PyObject_GetTypeData_GCSafe (doesn’t set exceptions)
PyObject_GetItemData_GCSafe (ditto)
PyType_GetModuleState_GCSafe (ditto)
PyModule_GetState_GCSafe (ditto)
PyModule_GetToken_GCSafe (ditto)
PyType_GetBaseByToken_GCSafe (doesn’t set exceptions & returns borrowed ref)
PyType_GetModule_GCSafe (ditto)
PyType_GetModuleByToken_GCSafe (ditto)
Additionally, document Py_TYPE & Py_SIZE [edit: and PyObject_VisitManagedDict] as GC-safe.
I guess that these new variants should be discouraged outside of use during GC because the borrowed references (and lack of exceptions) are generally worse for UX? Should the name be even stronger, maybe a TpTraverse suffix or prefix, e.g. PyObject_GetTypeData_TpTraverse, or PyTpTraverse_Object_GetTypeData? This way it’s more clear (to me) these functions are only for that use case, because they’re so specialized it probably doesn’t matter if their names are long.
At the risk of pointing out the something that’s absolutely obvious and trivial: PyObject_VisitManagedDict is hopefully GC-safe (and looks to be documented as such).
As for UX: I expect users to define their own helpers on top of these (most importantly, to cast the result PyModule_GetState). Defining two variants is, unfortunately, worse for UX than borrowed refs & lack of exceptions.
What I’m more worried about is borrowed references themselves. Without GIL, if another thread changes a class’s MRO, the PyType_GetBase* could become invalid.
This currently can’t happen: both GC and MRO changes are stop-the-world operations. But that doesn’t seem like something we want to guarantee in the long term.
Maybe the stop-the-world part is the key aspect and the part that should be highlighted in the name?
I’m not sure if the restrictions you’re thinking of are exactly the same as the ones for code happening during stop-the-world events, but it sounds similar to what @nas described to me when he was doing work to make mutating type objects thread-safe.
I suspect other profilers will end up using the private API for stopping the world since it’s needed to do prevent races while observing the Python stack. Better to come up with defined and documented semantics for doing this stuff.
As @encukou says at the end there, we probably don’t want to guarantee stop-the-world in the long-term as it’s really an implementation detail that could change. I’d like to keep the door open for a different design for the GC that may not be stop-the-world.
In #142376 we have a request for allowing PyObject_GetTypeDatawithout an attached thread state. It looks like the use case would apply for all the API listed here, which suggests that it might make sense to include that in the design.
CPython guarantee that the APIs are strictly read-only; they can’t change refcounts or create new objects. This makes them safe to use in tp_traverse.
CPython requires callers to ensure that the data the API “touches” isn’t concurrently modified. That data being:
the type of any PyObject* argument
the MRO of PyTypeObject* argument
for completeness: type layout data – flags, basicsize, itemsize, and similar. (Ideally we’d tell people to always treat those as immutable, but I don’t think we currently do…)
And we can let users ensure that by not changing it themselves and:
Call the functions from tp_traverse, or
Have an attached thread state (which locks CPython into changing type/bases being a stop-the-world operation), or
(to address #142376:) Use some new API to acquire a “reader lock on types & memory layouts” – instead of an attached thread state, you’d get a lock that prevents stop-the-world operations [2], but while you hold it, you’re only allowed to use the “_GCSafe” API. (I’m assuming that this could be significantly cheaper than attaching a thread state.)
We also need to outlaw directly setting ob_type, tp_bases, tp_basicsize etc., but I assume that’s already part of making an extension free-threading-compatible.
Does that sound like a reasonable direction?
it needs a better name; let me stick with this for now ↩︎
I’m not convinced these are worth the complexity of adding new functions, but I don’t feel super strongly.
I guess the idea is that they’re not safe if they are called incorrectly and set an exception, but:
Just don’t call them incorrectly. These exceptions are all of the “programmer error” type, not runtime error.
Pretty much nobody checks the exception anyways. They’re useless.
We’ve made our lives more difficult by making these simple functions capable of failing. I think the idea is that validating arguments and raising a Python error is somehow nicer for C API users than a C assertion or fatal error, but that totally ignores how people actually use these APIs.
Fair, I partially agree. There was a long discussion on fallible APIs; I don’t think we need to rehash it at this point. If fallibility was the only issue, I think we could make things work with the current API.
But the main issue is the borrowed references.
In “normal” code, another thread can invalidate borrowed refs; in tp_traverse we can’t create strong refs.
As the requester of the thread-state-less PyObject_GetTypeData this is going to sound a bit hypocritical. However:
What I’m a bit worried about is that the rules are very tied to “these things are done with stop the world” (which @emmatyping says, does feel like a real implementation detail).
The opaque type data is mainly useful for the Limited API, where ideally we want to stay away from implementation details.
Personally, I could easily live with restricting access to immutable types only. For those, the idea that the type and layout shouldn’t change really isn’t an implementation detail.
I think that’s just PyType_GetBaseByToken and PyType_GetModuleByToken. It seems pretty rare to need to call them from a tp_traverse handler. If really necessary, I think you can work around that in extensions by calling other APIs.
If there were a few extensions that would make use of it, I’d say to add those two with _Borrow or _GCSafe suffixes, but I’m not sure it’s worth it right now.
I strongly agree with this. The only call mentioned that affects me is PyModule_GetState() which can only fail if the argument is invalid. The fact that I “should” be checking the result is a pain. I would much rather have something that causes an abort as it’s a sign of a serious programming error on my part.
For those cases where code genuinely doesn’t know if there should be state or not then add PyModule_HasState().
I don’t think you can, if targetting ABIs with different sizeof(PyObject) (i.e. both free-threaded & GIL-ful).
For a concrete example, see here. I’d be interested in how you’d write design this.
IMO, “GC safe” needs to be a new concept/convention. It is a much stricter limitation than “cannot fail” or “unchecked”. For example, “GCSafe” functions can’t call “normal” functions and clear any exceptions.
And they need to be guaranteed to never poke at refcounts, even in future CPython versions. If nothing else, a _GCSafe suffix is a good way to remind future core devs of this limitation.
Thanks for your input!
Agree that our lives are more difficult, but, adding new API is a solution :)
The main general argument for making long-term-supported functions capable of failing is to enable them to emit runtime deprecation warnings (which -Werror turns into exceptions).
The C API WG policy is*[1] that new functions should be regular/predictable (be fallible, return new references, etc.), and we add a special variant (Unchecked, Borrow, etc.) if needed for performance, ergonomics, etc.
In my (obviously biased) opinion, this is a great way to design the API. While it does makes the individual functions more cumbersome, the API as a whole will be much more usable. (Especially if/when we can optionally hide the irregularities added in the past.)
I’m only a single member of the WG, so I shouldn’t really speak for all of it. I’m the one that’s writing the rules down though. ↩︎