Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
gh-118362: Fix thread safety around lookups from the type cache in the face of concurrent mutators#118454
Conversation
c0e757c toee74503CompareMakes setting an attribute on a class and signaling type modified atomicAvoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated
colesbury left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some questions comments below. Biggest questions is that now that we are accessing thedict 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
DinoV commentedMay 1, 2024
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 formattingAdd news blurbUse PyDictObject * moreRename _PyDict_GetItemRef_LockHeld to _PyDict_GetItemRef_Unicode_LockHeldReduce iterations in testExpose _PyObject_SetAttributeErrorContext for attaching context
DinoV commentedMay 1, 2024
|
Include/internal/pycore_object.h Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| externint_PyObject_SetAttributeErrorContext(PyObject*v,PyObject*name); | |
| externint_PyObject_SetAttributeErrorContext(PyObject*v,PyObject*name); |
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think thePyErr_Occurred() check below can be removed now.
colesbury left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM with a few minor issues:
- We need to handle the
PyDict_New()returningNULLcase intype_setattro - Minor formatting in
_PyObject_SetAttributeErrorContext PyErr_Occurred()case is no longer need infind_name_in_mro
bedevere-bot commentedMay 6, 2024
|
| class A: | ||
| attr = 1 | ||
| @unittest.skipIf(is_wasi, "WASI has no threads.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, probably better to use@threading_helper.requires_working_threading()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
iOS and Android do both have working threading, but they don't support subprocesses. In theorymultiprocessing.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 useconcurrent.futures.ThreadPoolExecutor instead.
… in the face of concurrent mutators (python#118454)Add _PyType_LookupRef and use incref before setting attribute on typeMakes setting an attribute on a class and signaling type modified atomicAvoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated
Python 3.14 introduced a new assertion that prevents us from usingPyObject_GenericSetAttr directly in our meta type. To work aroundthis, we manipulate the type dict directly.This workaround is a simplified variant of Cython's workaround fromcython/cython#6325.The relevant Python change is inpython/cpython#118454
Python 3.14 introduced a new assertion that prevents us from usingPyObject_GenericSetAttr directly in our meta type. To work aroundthis, we manipulate the type dict directly.This workaround is a simplified variant of Cython's workaround fromcython/cython#6325.The relevant Python change is inpython/cpython#118454
Python 3.14 introduced a new assertion that prevents us from usingPyObject_GenericSetAttr directly in our meta type. To work aroundthis, we manipulate the type dict directly.This workaround is a simplified variant of Cython's workaround fromcython/cython#6325.The relevant Python change is inpython/cpython#118454
Python 3.14 introduced a new assertion that prevents us from usingPyObject_GenericSetAttr directly in our meta type. To work aroundthis, we manipulate the type dict directly.This workaround is a simplified variant of Cython's workaround fromcython/cython#6325.The relevant Python change is inpython/cpython#118454
Python 3.14 introduced a new assertion that prevents us from usingPyObject_GenericSetAttr directly in our meta type. To work aroundthis, we manipulate the type dict directly.This workaround is a simplified variant of Cython's workaround fromcython/cython#6325.The relevant Python change is inpython/cpython#118454
Python 3.14 introduced a new assertion that prevents us from usingPyObject_GenericSetAttr directly in our meta type. To work aroundthis, we manipulate the type dict directly.This workaround is a simplified variant of Cython's workaround fromcython/cython#6325.The relevant Python change is inpython/cpython#118454
Python 3.14 introduced a new assertion that prevents us from usingPyObject_GenericSetAttr directly in our meta type. To work aroundthis, we manipulate the type dict directly.This workaround is a simplified variant of Cython's workaround fromcython/cython#6325.The relevant Python change is inpython/cpython#118454
Uh oh!
There was an error while loading.Please reload this page.
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.