mirror of
https://github.com/python/cpython.git
synced 2026-10-28 10:25:05 +08:00
gh-158197: Fix split dict iteration during concurrent deletion (gh-158686)
Read insertion order from the iterator's captured values snapshot instead of re-reading ma_values and asserting against a concurrently changing size. Retry under the dictionary lock when a value slot has been cleared. Make insertion-order writes atomic and publish size changes with release stores, pairing with the iterator's size load. Keep order-byte accesses relaxed. Add _Py_atomic_store_uint8_release() and FT_ATOMIC_STORE_UINT8_RELEASE() for this.
This commit is contained in:
1 parent
e5e0748abd
commit
4bb1dadff4
9 files changed
+129
-7
No files matched your search
@@ -526,6 +526,9 @@ _Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value);
|
||||
static inline void
|
||||
_Py_atomic_store_int8_release(int8_t *obj, int8_t value);
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value);
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_int_release(int *obj, int value);
|
||||
|
||||
|
||||
@@ -576,6 +576,10 @@ static inline void
|
||||
_Py_atomic_store_int8_release(int8_t *obj, int8_t value)
|
||||
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value)
|
||||
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value)
|
||||
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
|
||||
|
||||
@@ -1073,6 +1073,19 @@ _Py_atomic_store_int8_release(int8_t *obj, int8_t value)
|
||||
#endif
|
||||
}
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value)
|
||||
{
|
||||
#if defined(_M_X64) || defined(_M_IX86)
|
||||
*(uint8_t volatile *)obj = value;
|
||||
#elif defined(_M_ARM64)
|
||||
_Py_atomic_ASSERT_ARG_TYPE(unsigned __int8);
|
||||
__stlr8((unsigned __int8 volatile *)obj, (unsigned __int8)value);
|
||||
#else
|
||||
# error "no implementation of _Py_atomic_store_uint8_release"
|
||||
#endif
|
||||
}
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_uint_release(unsigned int *obj, unsigned int value)
|
||||
{
|
||||
|
||||
@@ -1031,6 +1031,14 @@ _Py_atomic_store_int8_release(int8_t *obj, int8_t value)
|
||||
memory_order_release);
|
||||
}
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value)
|
||||
{
|
||||
_Py_USING_STD;
|
||||
atomic_store_explicit((_Atomic(uint8_t)*)obj, value,
|
||||
memory_order_release);
|
||||
}
|
||||
|
||||
static inline void
|
||||
_Py_atomic_store_uint_release(unsigned int *obj, unsigned int value)
|
||||
{
|
||||
|
||||
@@ -354,8 +354,8 @@ _PyDictValues_AddToInsertionOrder(PyDictValues *values, Py_ssize_t ix)
|
||||
uint8_t *array = get_insertion_order_array(values);
|
||||
assert(size < values->capacity);
|
||||
assert(((uint8_t)ix) == ix);
|
||||
array[size] = (uint8_t)ix;
|
||||
values->size = size+1;
|
||||
FT_ATOMIC_STORE_UINT8_RELAXED(array[size], (uint8_t)ix);
|
||||
FT_ATOMIC_STORE_UINT8_RELEASE(values->size, size+1);
|
||||
}
|
||||
|
||||
// Exported for external JIT support
|
||||
|
||||
@@ -69,6 +69,8 @@ extern "C" {
|
||||
_Py_atomic_store_ssize_relaxed(&value, new_value)
|
||||
#define FT_ATOMIC_STORE_SSIZE_RELEASE(value, new_value) \
|
||||
_Py_atomic_store_ssize_release(&value, new_value)
|
||||
#define FT_ATOMIC_STORE_UINT8_RELEASE(value, new_value) \
|
||||
_Py_atomic_store_uint8_release(&value, new_value)
|
||||
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) \
|
||||
_Py_atomic_store_uint8_relaxed(&value, new_value)
|
||||
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) \
|
||||
@@ -167,6 +169,7 @@ extern "C" {
|
||||
#define FT_ATOMIC_STORE_INT8_RELEASE(value, new_value) value = new_value
|
||||
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value
|
||||
#define FT_ATOMIC_STORE_SSIZE_RELEASE(value, new_value) value = new_value
|
||||
#define FT_ATOMIC_STORE_UINT8_RELEASE(value, new_value) value = new_value
|
||||
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value
|
||||
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value
|
||||
#define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value
|
||||
|
||||
@@ -314,6 +314,87 @@ class TestDict(TestCase):
|
||||
|
||||
threading_helper.run_concurrently([writer, reader, reader])
|
||||
|
||||
def test_racing_split_dict_iteration_and_delete(self):
|
||||
# Each reader owns its iterator. Mortal values exercise reference
|
||||
# acquisition as well as the concurrent clearing of value slots.
|
||||
class C:
|
||||
pass
|
||||
|
||||
names = [f"a{i}" for i in range(8)]
|
||||
obj = C()
|
||||
for i, name in enumerate(names):
|
||||
setattr(obj, name, [i])
|
||||
d = obj.__dict__
|
||||
|
||||
def delattr_writer():
|
||||
for _ in range(50):
|
||||
for i, name in enumerate(names):
|
||||
delattr(obj, name)
|
||||
setattr(obj, name, [i])
|
||||
|
||||
def delitem_writer():
|
||||
for _ in range(50):
|
||||
for i, name in enumerate(names):
|
||||
del d[name]
|
||||
d[name] = [i]
|
||||
|
||||
def clear_writer():
|
||||
for _ in range(50):
|
||||
d.clear()
|
||||
for i, name in enumerate(names):
|
||||
setattr(obj, name, [i])
|
||||
|
||||
def reader(view):
|
||||
for _ in range(200):
|
||||
try:
|
||||
for _ in view():
|
||||
pass
|
||||
except RuntimeError:
|
||||
pass
|
||||
|
||||
for writer in (delattr_writer, delitem_writer, clear_writer):
|
||||
for view in (d.values, d.items, d.keys):
|
||||
with self.subTest(writer=writer.__name__, view=view.__name__):
|
||||
threading_helper.run_concurrently(
|
||||
[writer, partial(reader, view), partial(reader, view)])
|
||||
self.assertEqual(d, {name: [i] for i, name in enumerate(names)})
|
||||
|
||||
def test_racing_nonembedded_split_dict_iteration_and_delete(self):
|
||||
class C:
|
||||
pass
|
||||
|
||||
names = [f"a{i}" for i in range(8)]
|
||||
# Populate shared keys without populating the copied dict's order
|
||||
# array, so the writer also initializes previously unused order bytes.
|
||||
template = C()
|
||||
for i, name in enumerate(names):
|
||||
setattr(template, name, [i])
|
||||
|
||||
def writer():
|
||||
for _ in range(50):
|
||||
for i, name in enumerate(names):
|
||||
d.pop(name, None)
|
||||
d[name] = [i]
|
||||
|
||||
def reader(view):
|
||||
for _ in range(200):
|
||||
try:
|
||||
for _ in view():
|
||||
pass
|
||||
except RuntimeError:
|
||||
pass
|
||||
|
||||
for view_name in ("values", "items", "keys"):
|
||||
with self.subTest(view=view_name):
|
||||
obj = C()
|
||||
obj.a0 = [0]
|
||||
# Copying a split dict gives it heap-backed values.
|
||||
d = obj.__dict__.copy()
|
||||
view = getattr(d, view_name)
|
||||
threading_helper.run_concurrently(
|
||||
[writer, partial(reader, view), partial(reader, view)])
|
||||
self.assertEqual(d, {name: [i] for i, name in enumerate(names)})
|
||||
|
||||
def test_racing_dict_update_and_method_lookup(self):
|
||||
# gh-144295: test race between dict modifications and method lookups.
|
||||
# Uses BytesIO because the race requires a type without Py_TPFLAGS_INLINE_VALUES
|
||||
|
||||
@@ -0,0 +1,5 @@
|
||||
Fix crashes and debug-build assertion failures in the :term:`free-threaded
|
||||
build` when iterating over a split dictionary while another thread deletes
|
||||
entries or clears it. Publish insertion-order updates atomically so readers
|
||||
cannot observe an increased size before the corresponding order entry is
|
||||
initialized.
|
||||
+10
-5
@@ -2925,9 +2925,9 @@ delete_index_from_values(PyDictValues *values, Py_ssize_t ix)
|
||||
assert(i < size);
|
||||
size--;
|
||||
for (; i < size; i++) {
|
||||
array[i] = array[i+1];
|
||||
FT_ATOMIC_STORE_UINT8_RELAXED(array[i], array[i+1]);
|
||||
}
|
||||
values->size = size;
|
||||
FT_ATOMIC_STORE_UINT8_RELEASE(values->size, size);
|
||||
}
|
||||
|
||||
static void
|
||||
@@ -3122,7 +3122,7 @@ clear_embedded_values(PyDictValues *values, Py_ssize_t nentries)
|
||||
refs[i] = values->values[i];
|
||||
FT_ATOMIC_STORE_PTR_RELEASE(values->values[i], NULL);
|
||||
}
|
||||
values->size = 0;
|
||||
FT_ATOMIC_STORE_UINT8_RELEASE(values->size, 0);
|
||||
for (Py_ssize_t i = 0; i < nentries; i++) {
|
||||
Py_XDECREF(refs[i]);
|
||||
}
|
||||
@@ -6103,9 +6103,14 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self,
|
||||
// We're racing against writes to the order from delete_index_from_values, but
|
||||
// single threaded can suffer from concurrent modification to those as well and
|
||||
// can have either duplicated or skipped attributes, so we strive to do no better
|
||||
// here.
|
||||
int index = get_index_from_order(d, i);
|
||||
// here. Use the same values snapshot as the size load above.
|
||||
uint8_t *order = get_insertion_order_array(values);
|
||||
int index = _Py_atomic_load_uint8_relaxed(&order[i]);
|
||||
PyObject *value = _Py_atomic_load_ptr(&values->values[index]);
|
||||
if (value == NULL) {
|
||||
// Deletion clears the slot before decrementing values->size.
|
||||
goto try_locked;
|
||||
}
|
||||
if (acquire_key_value(&DK_UNICODE_ENTRIES(k)[index].me_key, value,
|
||||
&values->values[index], out_key, out_value) < 0) {
|
||||
goto try_locked;
|
||||
|
||||
Reference in new issue
Block a user