Skip to content

Commit 6418581

Browse files
committed
gh-158197: Fix split dict iteration during concurrent deletion
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.
1 parent 1b015e6 commit 6418581

9 files changed

Lines changed: 129 additions & 7 deletions

File tree

‎Include/cpython/pyatomic.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,9 @@ _Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value);
526526
static inline void
527527
_Py_atomic_store_int8_release(int8_t *obj, int8_t value);
528528

529+
static inline void
530+
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value);
531+
529532
static inline void
530533
_Py_atomic_store_int_release(int *obj, int value);
531534

‎Include/cpython/pyatomic_gcc.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,10 @@ static inline void
576576
_Py_atomic_store_int8_release(int8_t *obj, int8_t value)
577577
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
578578

579+
static inline void
580+
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value)
581+
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
582+
579583
static inline void
580584
_Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value)
581585
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }

‎Include/cpython/pyatomic_msc.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1073,6 +1073,19 @@ _Py_atomic_store_int8_release(int8_t *obj, int8_t value)
10731073
#endif
10741074
}
10751075

1076+
static inline void
1077+
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value)
1078+
{
1079+
#if defined(_M_X64) || defined(_M_IX86)
1080+
*(uint8_t volatile *)obj = value;
1081+
#elif defined(_M_ARM64)
1082+
_Py_atomic_ASSERT_ARG_TYPE(unsigned __int8);
1083+
__stlr8((unsigned __int8 volatile *)obj, (unsigned __int8)value);
1084+
#else
1085+
# error "no implementation of _Py_atomic_store_uint8_release"
1086+
#endif
1087+
}
1088+
10761089
static inline void
10771090
_Py_atomic_store_uint_release(unsigned int *obj, unsigned int value)
10781091
{

‎Include/cpython/pyatomic_std.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,14 @@ _Py_atomic_store_int8_release(int8_t *obj, int8_t value)
10311031
memory_order_release);
10321032
}
10331033

1034+
static inline void
1035+
_Py_atomic_store_uint8_release(uint8_t *obj, uint8_t value)
1036+
{
1037+
_Py_USING_STD;
1038+
atomic_store_explicit((_Atomic(uint8_t)*)obj, value,
1039+
memory_order_release);
1040+
}
1041+
10341042
static inline void
10351043
_Py_atomic_store_uint_release(unsigned int *obj, unsigned int value)
10361044
{

‎Include/internal/pycore_dict.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -354,8 +354,8 @@ _PyDictValues_AddToInsertionOrder(PyDictValues *values, Py_ssize_t ix)
354354
uint8_t *array = get_insertion_order_array(values);
355355
assert(size < values->capacity);
356356
assert(((uint8_t)ix) == ix);
357-
array[size] = (uint8_t)ix;
358-
values->size = size+1;
357+
FT_ATOMIC_STORE_UINT8_RELAXED(array[size], (uint8_t)ix);
358+
FT_ATOMIC_STORE_UINT8_RELEASE(values->size, size+1);
359359
}
360360

361361
// Exported for external JIT support

‎Include/internal/pycore_pyatomic_ft_wrappers.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ extern "C" {
6969
_Py_atomic_store_ssize_relaxed(&value, new_value)
7070
#define FT_ATOMIC_STORE_SSIZE_RELEASE(value, new_value) \
7171
_Py_atomic_store_ssize_release(&value, new_value)
72+
#define FT_ATOMIC_STORE_UINT8_RELEASE(value, new_value) \
73+
_Py_atomic_store_uint8_release(&value, new_value)
7274
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) \
7375
_Py_atomic_store_uint8_relaxed(&value, new_value)
7476
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) \
@@ -167,6 +169,7 @@ extern "C" {
167169
#define FT_ATOMIC_STORE_INT8_RELEASE(value, new_value) value = new_value
168170
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value
169171
#define FT_ATOMIC_STORE_SSIZE_RELEASE(value, new_value) value = new_value
172+
#define FT_ATOMIC_STORE_UINT8_RELEASE(value, new_value) value = new_value
170173
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value
171174
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value
172175
#define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value

‎Lib/test/test_free_threading/test_dict.py‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,87 @@ def reader():
314314

315315
threading_helper.run_concurrently([writer, reader, reader])
316316

317+
def test_racing_split_dict_iteration_and_delete(self):
318+
# Each reader owns its iterator. Mortal values exercise reference
319+
# acquisition as well as the concurrent clearing of value slots.
320+
class C:
321+
pass
322+
323+
names = [f"a{i}" for i in range(8)]
324+
obj = C()
325+
for i, name in enumerate(names):
326+
setattr(obj, name, [i])
327+
d = obj.__dict__
328+
329+
def delattr_writer():
330+
for _ in range(50):
331+
for i, name in enumerate(names):
332+
delattr(obj, name)
333+
setattr(obj, name, [i])
334+
335+
def delitem_writer():
336+
for _ in range(50):
337+
for i, name in enumerate(names):
338+
del d[name]
339+
d[name] = [i]
340+
341+
def clear_writer():
342+
for _ in range(50):
343+
d.clear()
344+
for i, name in enumerate(names):
345+
setattr(obj, name, [i])
346+
347+
def reader(view):
348+
for _ in range(200):
349+
try:
350+
for _ in view():
351+
pass
352+
except RuntimeError:
353+
pass
354+
355+
for writer in (delattr_writer, delitem_writer, clear_writer):
356+
for view in (d.values, d.items, d.keys):
357+
with self.subTest(writer=writer.__name__, view=view.__name__):
358+
threading_helper.run_concurrently(
359+
[writer, partial(reader, view), partial(reader, view)])
360+
self.assertEqual(d, {name: [i] for i, name in enumerate(names)})
361+
362+
def test_racing_nonembedded_split_dict_iteration_and_delete(self):
363+
class C:
364+
pass
365+
366+
names = [f"a{i}" for i in range(8)]
367+
# Populate shared keys without populating the copied dict's order
368+
# array, so the writer also initializes previously unused order bytes.
369+
template = C()
370+
for i, name in enumerate(names):
371+
setattr(template, name, [i])
372+
373+
def writer():
374+
for _ in range(50):
375+
for i, name in enumerate(names):
376+
d.pop(name, None)
377+
d[name] = [i]
378+
379+
def reader(view):
380+
for _ in range(200):
381+
try:
382+
for _ in view():
383+
pass
384+
except RuntimeError:
385+
pass
386+
387+
for view_name in ("values", "items", "keys"):
388+
with self.subTest(view=view_name):
389+
obj = C()
390+
obj.a0 = [0]
391+
# Copying a split dict gives it heap-backed values.
392+
d = obj.__dict__.copy()
393+
view = getattr(d, view_name)
394+
threading_helper.run_concurrently(
395+
[writer, partial(reader, view), partial(reader, view)])
396+
self.assertEqual(d, {name: [i] for i, name in enumerate(names)})
397+
317398
def test_racing_dict_update_and_method_lookup(self):
318399
# gh-144295: test race between dict modifications and method lookups.
319400
# Uses BytesIO because the race requires a type without Py_TPFLAGS_INLINE_VALUES
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix crashes and debug-build assertion failures in the :term:`free-threaded
2+
build` when iterating over a split dictionary while another thread deletes
3+
entries or clears it. Publish insertion-order updates atomically so readers
4+
cannot observe an increased size before the corresponding order entry is
5+
initialized.

‎Objects/dictobject.c‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2925,9 +2925,9 @@ delete_index_from_values(PyDictValues *values, Py_ssize_t ix)
29252925
assert(i < size);
29262926
size--;
29272927
for (; i < size; i++) {
2928-
array[i] = array[i+1];
2928+
FT_ATOMIC_STORE_UINT8_RELAXED(array[i], array[i+1]);
29292929
}
2930-
values->size = size;
2930+
FT_ATOMIC_STORE_UINT8_RELEASE(values->size, size);
29312931
}
29322932

29332933
static void
@@ -3122,7 +3122,7 @@ clear_embedded_values(PyDictValues *values, Py_ssize_t nentries)
31223122
refs[i] = values->values[i];
31233123
FT_ATOMIC_STORE_PTR_RELEASE(values->values[i], NULL);
31243124
}
3125-
values->size = 0;
3125+
FT_ATOMIC_STORE_UINT8_RELEASE(values->size, 0);
31263126
for (Py_ssize_t i = 0; i < nentries; i++) {
31273127
Py_XDECREF(refs[i]);
31283128
}
@@ -6102,9 +6102,14 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self,
61026102
// We're racing against writes to the order from delete_index_from_values, but
61036103
// single threaded can suffer from concurrent modification to those as well and
61046104
// can have either duplicated or skipped attributes, so we strive to do no better
6105-
// here.
6106-
int index = get_index_from_order(d, i);
6105+
// here. Use the same values snapshot as the size load above.
6106+
uint8_t *order = get_insertion_order_array(values);
6107+
int index = _Py_atomic_load_uint8_relaxed(&order[i]);
61076108
PyObject *value = _Py_atomic_load_ptr(&values->values[index]);
6109+
if (value == NULL) {
6110+
// Deletion clears the slot before decrementing values->size.
6111+
goto try_locked;
6112+
}
61086113
if (acquire_key_value(&DK_UNICODE_ENTRIES(k)[index].me_key, value,
61096114
&values->values[index], out_key, out_value) < 0) {
61106115
goto try_locked;

0 commit comments

Comments
 (0)