Uh oh!
There was an error while loading. Please reload this page.
gh-112075: Fix race in constructing dict for instance - #118499
Conversation
colesbury
left a comment
There was a problem hiding this comment.
We also need to fix:
- PyObject_GenericGetDict (non-managed dict case)
- PyObject_GenericSetDict
| assert(dictptr != NULL); | ||
| if ((tp->tp_flags & Py_TPFLAGS_HEAPTYPE) && (cached = CACHED_KEYS(tp))) { | ||
| assert(dictptr != NULL); | ||
| dict = *dictptr; |
There was a problem hiding this comment.
We need to use atomics to load from the dictptr or there will be a data race between the read here and the write within the critical section.
a9ad363 to
56a79e6Compare| Py_BEGIN_CRITICAL_SECTION(dict); | ||
| res = set_or_del_lock_held((PyDictObject *)dict, key, value); | ||
| Py_END_CRITICAL_SECTION(); |
There was a problem hiding this comment.
I think the ASSERT_CONSISTENT needs to be inside the critical section.
| { | ||
| PyDictKeysObject *cached; | ||
| PyObject *dict = FT_ATOMIC_LOAD_PTR_RELAXED(*dictptr); |
| else { | ||
| dict = PyDict_New(); | ||
| } | ||
| FT_ATOMIC_STORE_PTR_RELAXED(*dictptr, dict); |
| if (dict == NULL) { | ||
| dictkeys_decref(interp, cached, false); | ||
| } |
There was a problem hiding this comment.
I think new_dict_with_shared_keys already handles the dictkeys_decref in the error case.
There was a problem hiding this comment.
Oh, that's super subtle, I don't think there's any reason to spread this out, I'll move the incref into new_dict_with_shared_keys
| return -1; | ||
| } | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_XDECREF(_Py_atomic_exchange_ptr(dictptr, Py_NewRef(value))); |
There was a problem hiding this comment.
I think we need to be consistent about either using atomic exchange/cas or using the object locks to protect the dictptr. I don't think we can safely mix them.
| } | ||
| #endif | ||
| PyTypeObject *tp = Py_TYPE(obj); | ||
| if ((tp->tp_flags & Py_TPFLAGS_HEAPTYPE) && (cached = CACHED_KEYS(tp))) { |
There was a problem hiding this comment.
maybe: _PyType_HasFeature(tp, Py_TPFLAGS_HEAPTYPE)
| goto done; | ||
| } | ||
| #endif | ||
| OBJECT_STAT_INC(dict_materialized_on_request); |
There was a problem hiding this comment.
Are we consistent about when we increment this stat?
There was a problem hiding this comment.
We seem to actually have an extra call in materialize_managed_dict_lock_held which is the inline values case. This one seems like maybe it shouldn't exist at all, as we no longer have a simple values array which is outside of the dict, so even though we have shared keys here we will always have a dict. I'll remove it from here.
colesbury
left a comment
There was a problem hiding this comment.
LGTM. Two minor comments below
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Lock obj instead of using atomic, and fix reads/writes
Fix a small thread safety issue w/ dicts. When we are constructing the dict for an object with either a non-inline managed dict or with a non-managed dict we're not properly protecting the creation with a lock. This locks the object in
_PyObjectDict_SetItemif we don't already have a dict to ensure only one thread creates the lock at a time.dictobjects thread-safe in--disable-gilbuilds #112075