gh-118362: Fix thread safety around lookups from the type cache in the face of concurrent mutators #118454
Conversation
DinoV
force-pushed
the
nogil_type_crash
branch
5 times, most recently
from
c0e757c to
ee74503
Compare
Makes setting an attribute on a class and signaling type modified atomic Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated
There was a problem hiding this comment.
Some questions comments below. Biggest questions is that now that we are accessing the dict directly instead of through _PyObject_GenericSetAttrWithDict, how do we know that the other cases handled by _PyObject_GenericSetAttrWithDict are not necessary in _Py_type_getattro?
Also, I think this merits a NEWS entry.
This is part of the reason why I assert: In type_setattro. Type objects will always have a dictptr, so we'll either go to
|
Fix some formatting Add news blurb Use PyDictObject * more Rename _PyDict_GetItemRef_LockHeld to _PyDict_GetItemRef_Unicode_LockHeld Reduce iterations in test Expose _PyObject_SetAttributeErrorContext for attaching context
|
colesbury
changed the title
Include/internal/pycore_object.h
Outdated
| @@ -658,6 +658,7 @@ extern PyObject *_PyType_NewManagedObject(PyTypeObject *type); | |||
| extern PyTypeObject* _PyType_CalculateMetaclass(PyTypeObject *, PyObject *); | |||
| extern PyObject* _PyType_GetDocFromInternalDoc(const char *, const char *); | |||
| extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const char *, int); | |||
| extern int _PyObject_SetAttributeErrorContext(PyObject* v, PyObject* name); | |||
There was a problem hiding this comment.
| extern int _PyObject_SetAttributeErrorContext(PyObject* v, PyObject* name); | |
| extern int _PyObject_SetAttributeErrorContext(PyObject *v, PyObject *name); |
| @@ -4981,7 +5026,10 @@ find_name_in_mro(PyTypeObject *type, PyObject *name, int *error) | |||
| PyObject *base = PyTuple_GET_ITEM(mro, i); | |||
| PyObject *dict = lookup_tp_dict(_PyType_CAST(base)); | |||
| assert(dict && PyDict_Check(dict)); | |||
| res = _PyDict_GetItem_KnownHash(dict, name, hash); | |||
| if (_PyDict_GetItemRef_KnownHash((PyDictObject *)dict, name, hash, &res) < 0) { | |||
There was a problem hiding this comment.
I think the PyErr_Occurred() check below can be removed now.
|
| class A: | ||
| attr = 1 | ||
|
|
||
| @unittest.skipIf(is_wasi, "WASI has no threads.") |
There was a problem hiding this comment.
Oh, probably better to use @threading_helper.requires_working_threading()
There was a problem hiding this comment.
iOS and Android do both have working threading, but they don't support subprocesses. In theory multiprocessing.dummy.Pool should work on these platforms, but in practice it doesn't because it imports too much of the main multiprocessing module.
The simplest solution is probably to use concurrent.futures.ThreadPoolExecutor instead.
… in the face of concurrent mutators (python#118454) Add _PyType_LookupRef and use incref before setting attribute on type Makes setting an attribute on a class and signaling type modified atomic Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated
Lookups from the type cache can be exposed to some races. One of those is that
_PyType_Lookupisn't increfing the result, so the resulting value can become invalid at any point. This adds a new_PyType_FetchAPI which does the incref. Most places are updated to use the new API except for the specializer which isn't safe in free-threaded builds yet.But there are also issues around when we do a store into the dict and signal that the type is modified. A race can occur where the old value is removed from the dict and before we invalidate the type version. This can be seen today w/ the GIL when the value in the dict has a finalizer - accessing the type cache and the type's mapping proxy return inconsistent results.
This fixes these issues by making the update to the dict and the type modified update an atomic operation. We also capture the previous value in the dictionary and don't free it until after we've made the atomic update. This basically inlines a much simplified version of
_PyObject_GenericSetAttrWithDictintotype_setattrowhich can lock mutation against types as well as against the type's dict.When updating we first clear the type version so concurrent reads of the type cache won't hit. We then replace the existing value and finish the type modified process.