GH-129817: thread safety for tp_flags - #130983
Conversation
Sorry, something went wrong.
Use separate structure member in PyHeapTypeObject, `ht_flags`, to store a copy of the type flags. That allows safe toggling of some of the flags after the type has been exposed.
colesbury
left a comment
There was a problem hiding this comment.
This seems related to the following issue in that some of the operations that change tp_flags also change type slots:
Type slots are not thread-safe in free-threaded builds (#127266)
Overall, I'm not really sure about this:
- I think these sorts of changes are more likely to introduce problems than fix any problems users will actually encounter
- The abc related flags seem like things that are going to be set during or immediately after type creation. I'm not convinced we should spend effort making that thread-safe
Sorry, something went wrong.
| // Non-heap types are immutable and so these flags can only be toggled | ||
| // after creation on heap types. |
There was a problem hiding this comment.
I'm a bit worried about this assumption.
Sorry, something went wrong.
There was a problem hiding this comment.
Do you worry that it's not currently true or that it might be not true in a future version of Python? The fact that non-heap types are immutable seems a pretty important assumption currently. In PyType_Ready():
/* Historically, all static types were immutable. See bpo-43908 */
if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
type_add_flags(type, Py_TPFLAGS_IMMUTABLETYPE);
/* Static types must be immortal */
_Py_SetImmortalUntracked((PyObject *)type);
}
Sorry, something went wrong.
There was a problem hiding this comment.
I'm worried that it's not currently true:
- I don't really trust that code comment. Static types and C API extensions predate Py_TPFLAGS_IMMUTABLETYPE. I think you can support attribute modification on types if you construct your static type in the right way.
- Immutable in that context is talking about attribute modification, not changes to tp_flags. I don't think it covers things like collections.abc.Iterator.register(my_type)
- C extensions can modify type slots and tp_flags even if you can't change attributes from Python code.
Sorry, something went wrong.
Yeah, I think it's related. Doing a stop-the-world pause seems like the only reasonable fix for that. Maybe the same thing could be done for these couple of tp_flags that can be toggled after initial type creation?
I agree that it's pretty unlikely for users to actually encounter problems with this. As you say, the ABC register() is generally called soon after the type is created. It doesn't have to be but maybe we could add a documentation note somewhere that to be free-threading safe then register() needs to be called before the class is used. Aside from the flags to support ABC, the only other one is Py_TPFLAGS_HAVE_VECTORCALL. I think having that flag read wrongly would only cause your newly set __call__ method to be ignored (not cause crashes or other nasty behavior). Instead, Python would keep using vectorcall. We could fix this by rather than clearing the Py_TPFLAGS_HAVE_VECTORCALL flag we could set tp_vectorcall_offset to something invalid, like -1. Then, in places like _PyVectorcall_FunctionInline, check that tp_vectorcall_offset > 0. We can read and write tp_vectorcall_offset atomically.
Yeah, I think that's the case. In restrospect, we should have did the register() operation differently. E.g. when defining the class, you could have a special class attribute like __abc_types__ with a list of ABCs that should be registered. Then it can be done at type_ready() time. Another idea is to make _abc_register() do a stop-the-world pause. Registering new ABC types is not a very common operation. |
Sorry, something went wrong.
|
I think these are reasonable ideas. I'm still not sure they are worth pursuing right now. I'd be okay with deferring work on the potential issues with tp_flags and type slots until 3.15. |
Sorry, something went wrong.
|
Closing this for now, we can re-visit for 3.15. |
Sorry, something went wrong.
Note: this PR needs a bit more polish before it becomes non-draft. I added Sam and Matt to the reviewers in case they have some initial feedback on this approach.
Use a separate structure member in PyHeapTypeObject, ht_flags, to store a copy of the type flags. That allows safe toggling of some of the flags after the type has been initialized and potentially exposed to other threads.
We would prefer to not use an atomic load whenever tp_flags is read. That causes a lot of code churn (see gh-130892) and potentially some performance hit on platforms with weak memory ordering. Instead, we would like to use a normal load and only set the flags before the type has been exposed to other threads. That's mostly the case except for the flags listed below.
The following type flags cause issues in that they may be toggled after the type is initially created and exposed:
This PR does the following (only to the free-threaded build, the default build continues to work the same):
This approach causes a bit of extra complication since we have to check Py_TPFLAGS_HEAPTYPE first to know where to look at the flags. For non-heap types, they are in tp_flags always. This could be avoided by adding an additional member to PyTypeObject. However, I think the Py_TPFLAGS_HEAPTYPE check should be cheap enough and putting it in PyHeapTypeObject ensures that extension types don't get confused by it.
Note that since all non-heap types are immutable, it is not possible to toggle type flags for them (at least, CPython itself doesn't do so). Also note that this change can break extensions that manipulate tp_flags directly or directly tests them, rather than using functions.