From 1533d1df3a6deb0fb140df6f91d82f59dfd3737a Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Wed, 8 Jan 2025 15:30:14 +0100 Subject: [PATCH 01/11] Make list, tuple and range iteration more thread-safe, and actually test concurrent iteration. (This is prep work for enabling specialization of FOR_ITER in free-threaded builds.) The basic premise is: - Iterating over a shared _iterable_ (list, tuple or range) should be safe, not involve data races, and behave like iteration normally does. - Using a shared _iterator_ should not crash or involve data races, and should only produce items regular iteration would produce. It is _not_ guaranteed to produce all items, or produce each item only once. Providing stronger guarantees is possible for some of these iterators, but it's not always straight-forward and can significantly hamper the common case. Since iterators in general aren't shared between threads, and it's simply impossible to concurrently use many iterators (like generators), better to make sharing iterators without explicit synchronization clearly wrong. Specific issues fixed in order to make the tests pass: - List iteration could occasionally crash when a shared list wasn't already marked as shared when reallocated. - Tuple iteration could occasionally crash when the iterator's reference to the tuple was cleared on exhaustion. Like with list iteration, in free-threaded builds we can't safely and efficiently clear the iterator's reference to the iterable (doing it safely would mean extra, slow refcount operations), so just keep the iterable reference around. - Fast range iterators (for integers that fit in C longs) shared between threads would sometimes produce values past the end of the range, because the iterators use two bits of state that we can't efficiently update atomically. Rewriting the iterators to have a single bit of state is possible, but probably means more math for each iteration and may not be worth it. - Long range iterators (for other numbers) shared between threads would crash catastrophically in a variety of ways. This now uses a critical section. Rewriting this to be more efficient is probably possible, but since it deals with arbitrary Python objects it's difficult to get right. There seem to be no more exising races in list_get_item_ref, so drop it from the tsan suppression list. --- Include/internal/pycore_range.h | 6 + Lib/test/libregrtest/tsan.py | 1 + .../test_free_threading/test_iteration.py | 124 ++++++++++++++++++ Objects/listobject.c | 2 +- Objects/rangeobject.c | 91 +++++++++---- Objects/tupleobject.c | 9 +- Tools/tsan/suppressions_free_threading.txt | 1 - 7 files changed, 201 insertions(+), 33 deletions(-) create mode 100644 Lib/test/test_free_threading/test_iteration.py diff --git a/Include/internal/pycore_range.h b/Include/internal/pycore_range.h index bf045ec4fd8332..491c7297c9c05b 100644 --- a/Include/internal/pycore_range.h +++ b/Include/internal/pycore_range.h @@ -13,6 +13,12 @@ typedef struct { long start; long step; long len; +#ifdef Py_GIL_DISABLED + // Make sure multi-threaded use of a single iterator doesn't produce + // values past the end of the range (even though it is NOT guaranteed to + // uniquely produce all the values in the range!) + long stop; +#endif } _PyRangeIterObject; #ifdef __cplusplus diff --git a/Lib/test/libregrtest/tsan.py b/Lib/test/libregrtest/tsan.py index 00d5779d950e72..5f83cd03067274 100644 --- a/Lib/test/libregrtest/tsan.py +++ b/Lib/test/libregrtest/tsan.py @@ -26,6 +26,7 @@ 'test_threadsignals', 'test_weakref', 'test_free_threading.test_slots', + 'test_free_threading.test_iteration', ] diff --git a/Lib/test/test_free_threading/test_iteration.py b/Lib/test/test_free_threading/test_iteration.py new file mode 100644 index 00000000000000..9a88a1e03e36b0 --- /dev/null +++ b/Lib/test/test_free_threading/test_iteration.py @@ -0,0 +1,124 @@ +import sys +import threading +import unittest +from test import support + +# The race conditions these tests were written for only happen every now and +# then, even with the current numbers. To find rare race conditions, bumping +# these up will help, but it makes the test runtime highly variable under +# free-threading. Overhead is much higher under ThreadSanitizer, but it's +# also much better at detecting certain races, so we don't need as many +# items/threads. +if support.check_sanitizer(thread=True): + NUMITEMS = 1000 + NUMTHREADS = 2 +else: + NUMITEMS = 50000 + NUMTHREADS = 3 +NUMMUTATORS = 2 + +class ContendedTupleIterationTest(unittest.TestCase): + def make_testdata(self, n): + return tuple(range(n)) + + def assert_iterator_results(self, results, expected): + # Most iterators are not atomic (yet?) so they can skip or duplicate + # items, but they should not invent new items (like the range + # iterator has done in the past). + extra_items = set(results) - set(expected) + self.assertEqual(set(), extra_items) + + def run_threads(self, func, *args, numthreads=NUMTHREADS): + threads = [] + for _ in range(numthreads): + t = threading.Thread(target=func, args=args) + t.start() + threads.append(t) + return threads + + def test_iteration(self): + """Test iteration over a shared container""" + seq = self.make_testdata(NUMITEMS) + results = [] + start = threading.Event() + def worker(): + idx = 0 + start.wait() + for item in seq: + idx += 1 + results.append(idx) + threads = self.run_threads(worker) + start.set() + for t in threads: + t.join() + # Each thread has its own iterator, so results should be entirely predictable. + self.assertEqual(results, [NUMITEMS] * NUMTHREADS) + + def test_shared_iterator(self): + """Test iteration over a shared iterator""" + seq = self.make_testdata(NUMITEMS) + it = iter(seq) + results = [] + start = threading.Event() + def worker(): + items = [] + start.wait() + # We want a tight loop, so put items in the shared list at the end. + for item in it: + items.append(item) + results.extend(items) + threads = self.run_threads(worker) + start.set() + for t in threads: + t.join() + self.assert_iterator_results(sorted(results), seq) + +class ContendedListIterationTest(ContendedTupleIterationTest): + def make_testdata(self, n): + return list(range(n)) + + def test_iteration_while_mutating(self): + """Test iteration over a shared mutating container.""" + seq = self.make_testdata(NUMITEMS) + results = [] + start = threading.Event() + endmutate = threading.Event() + def mutator(): + orig = seq[:] + # Make changes big enough to cause resizing of the list, with + # items shifted around for good measure. + replacement = (orig * 3)[NUMITEMS//2:] + start.wait() + while not endmutate.is_set(): + seq[:] = replacement + seq[:] = orig + def worker(): + items = [] + start.wait() + # We want a tight loop, so put items in the shared list at the end. + for item in seq: + items.append(item) + results.extend(items) + mutators = () + try: + threads = self.run_threads(worker) + mutators = self.run_threads(mutator, numthreads=NUMMUTATORS) + start.set() + for t in threads: + t.join() + finally: + endmutate.set() + for m in mutators: + m.join() + self.assert_iterator_results(results, list(seq)) + + +class ContendedRangeIterationTest(ContendedTupleIterationTest): + def make_testdata(self, n): + return range(n) + + +class ContendedLongRangeIterationTest(ContendedTupleIterationTest): + def make_testdata(self, n): + return range(0, sys.maxsize*n, sys.maxsize) + diff --git a/Objects/listobject.c b/Objects/listobject.c index bbd53e7de94a31..2210ac6e9f68f0 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -344,7 +344,7 @@ list_item_impl(PyListObject *self, Py_ssize_t idx) static inline PyObject* list_get_item_ref(PyListObject *op, Py_ssize_t i) { - if (!_Py_IsOwnedByCurrentThread((PyObject *)op) && !_PyObject_GC_IS_SHARED(op)) { + if (!_Py_IsOwnedByCurrentThread((PyObject *)op)) { return list_item_impl(op, i); } // Need atomic operation for the getting size. diff --git a/Objects/rangeobject.c b/Objects/rangeobject.c index 2942ab624edf72..196838713c2e48 100644 --- a/Objects/rangeobject.c +++ b/Objects/rangeobject.c @@ -7,6 +7,7 @@ #include "pycore_modsupport.h" // _PyArg_NoKwnames() #include "pycore_range.h" #include "pycore_tuple.h" // _PyTuple_ITEMS() +#include "pycore_pyatomic_ft_wrappers.h" /* Support objects whose length is > PY_SSIZE_T_MAX. @@ -816,10 +817,19 @@ PyTypeObject PyRange_Type = { static PyObject * rangeiter_next(_PyRangeIterObject *r) { - if (r->len > 0) { - long result = r->start; - r->start = result + r->step; - r->len--; + long len = FT_ATOMIC_LOAD_LONG_RELAXED(r->len); + if (len > 0) { + long result = FT_ATOMIC_LOAD_LONG_RELAXED(r->start); + FT_ATOMIC_STORE_LONG_RELAXED(r->start, result + r->step); + FT_ATOMIC_STORE_LONG_RELAXED(r->len, len - 1); +#ifdef Py_GIL_DISABLED + // Concurrent calls can cause start to be updated by another thread + // after our len check, so double-check that we're not past the end. + if ((r->step > 0 && result >= r->stop) || + (r->step < 0 && result <= r->stop)) { + return NULL; + } +#endif return PyLong_FromLong(result); } return NULL; @@ -828,7 +838,7 @@ rangeiter_next(_PyRangeIterObject *r) static PyObject * rangeiter_len(_PyRangeIterObject *r, PyObject *Py_UNUSED(ignored)) { - return PyLong_FromLong(r->len); + return PyLong_FromLong(FT_ATOMIC_LOAD_LONG_RELAXED(r->len)); } PyDoc_STRVAR(length_hint_doc, @@ -841,10 +851,14 @@ rangeiter_reduce(_PyRangeIterObject *r, PyObject *Py_UNUSED(ignored)) PyObject *range; /* create a range object for pickling */ - start = PyLong_FromLong(r->start); + start = PyLong_FromLong(FT_ATOMIC_LOAD_LONG_RELAXED(r->start)); if (start == NULL) goto err; +#ifdef Py_GIL_DISABLED + stop = PyLong_FromLong(r->stop); +#else stop = PyLong_FromLong(r->start + r->len * r->step); +#endif if (stop == NULL) goto err; step = PyLong_FromLong(r->step); @@ -870,13 +884,15 @@ rangeiter_setstate(_PyRangeIterObject *r, PyObject *state) long index = PyLong_AsLong(state); if (index == -1 && PyErr_Occurred()) return NULL; + long len = FT_ATOMIC_LOAD_LONG_RELAXED(r->len); /* silently clip the index value */ if (index < 0) index = 0; - else if (index > r->len) - index = r->len; /* exhausted iterator */ - r->start += index * r->step; - r->len -= index; + else if (index > len) + index = len; /* exhausted iterator */ + FT_ATOMIC_STORE_LONG_RELAXED(r->start, + FT_ATOMIC_LOAD_LONG_RELAXED(r->start) + index * r->step); + FT_ATOMIC_STORE_LONG_RELAXED(r->len, len - index); Py_RETURN_NONE; } @@ -966,6 +982,9 @@ fast_range_iter(long start, long stop, long step, long len) it->start = start; it->step = step; it->len = len; +#ifdef Py_GIL_DISABLED + it->stop = stop; +#endif return (PyObject *)it; } @@ -979,36 +998,43 @@ typedef struct { static PyObject * longrangeiter_len(longrangeiterobject *r, PyObject *no_args) { - Py_INCREF(r->len); - return r->len; + PyObject *len; + Py_BEGIN_CRITICAL_SECTION(r); + len = Py_NewRef(r->len); + Py_END_CRITICAL_SECTION(); + return len; } static PyObject * longrangeiter_reduce(longrangeiterobject *r, PyObject *Py_UNUSED(ignored)) { PyObject *product, *stop=NULL; - PyObject *range; + PyObject *range, *result=NULL; + Py_BEGIN_CRITICAL_SECTION(r); /* create a range object for pickling. Must calculate the "stop" value */ product = PyNumber_Multiply(r->len, r->step); if (product == NULL) - return NULL; + goto fail; stop = PyNumber_Add(r->start, product); Py_DECREF(product); if (stop == NULL) - return NULL; + goto fail; range = (PyObject*)make_range_object(&PyRange_Type, Py_NewRef(r->start), stop, Py_NewRef(r->step)); if (range == NULL) { Py_DECREF(r->start); Py_DECREF(stop); Py_DECREF(r->step); - return NULL; + goto fail; } /* return the result */ - return Py_BuildValue("N(N)O", _PyEval_GetBuiltin(&_Py_ID(iter)), - range, Py_None); + result = Py_BuildValue("N(N)O", _PyEval_GetBuiltin(&_Py_ID(iter)), + range, Py_None); +fail: + Py_END_CRITICAL_SECTION(); + return result; } static PyObject * @@ -1016,38 +1042,43 @@ longrangeiter_setstate(longrangeiterobject *r, PyObject *state) { PyObject *zero = _PyLong_GetZero(); // borrowed reference int cmp; + PyObject *result = NULL; + Py_BEGIN_CRITICAL_SECTION(r); /* clip the value */ cmp = PyObject_RichCompareBool(state, zero, Py_LT); if (cmp < 0) - return NULL; + goto fail; if (cmp > 0) { state = zero; } else { cmp = PyObject_RichCompareBool(r->len, state, Py_LT); if (cmp < 0) - return NULL; + goto fail; if (cmp > 0) state = r->len; } PyObject *product = PyNumber_Multiply(state, r->step); if (product == NULL) - return NULL; + goto fail; PyObject *new_start = PyNumber_Add(r->start, product); Py_DECREF(product); if (new_start == NULL) - return NULL; + goto fail; PyObject *new_len = PyNumber_Subtract(r->len, state); if (new_len == NULL) { Py_DECREF(new_start); - return NULL; + goto fail; } PyObject *tmp = r->start; r->start = new_start; Py_SETREF(r->len, new_len); Py_DECREF(tmp); - Py_RETURN_NONE; + result = Py_NewRef(Py_None); +fail: + Py_END_CRITICAL_SECTION(); + return result; } static PyMethodDef longrangeiter_methods[] = { @@ -1072,21 +1103,25 @@ longrangeiter_dealloc(longrangeiterobject *r) static PyObject * longrangeiter_next(longrangeiterobject *r) { + PyObject *result = NULL; + Py_BEGIN_CRITICAL_SECTION(r); if (PyObject_RichCompareBool(r->len, _PyLong_GetZero(), Py_GT) != 1) - return NULL; + goto fail; PyObject *new_start = PyNumber_Add(r->start, r->step); if (new_start == NULL) { - return NULL; + goto fail; } PyObject *new_len = PyNumber_Subtract(r->len, _PyLong_GetOne()); if (new_len == NULL) { Py_DECREF(new_start); - return NULL; + goto fail; } - PyObject *result = r->start; + result = r->start; r->start = new_start; Py_SETREF(r->len, new_len); +fail: + Py_END_CRITICAL_SECTION(); return result; } diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index 49977726eadca9..c7d2a608a603c9 100644 --- a/Objects/tupleobject.c +++ b/Objects/tupleobject.c @@ -1017,14 +1017,17 @@ tupleiter_next(PyObject *obj) return NULL; assert(PyTuple_Check(seq)); - if (it->it_index < PyTuple_GET_SIZE(seq)) { - item = PyTuple_GET_ITEM(seq, it->it_index); - ++it->it_index; + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + if (index < PyTuple_GET_SIZE(seq)) { + item = PyTuple_GET_ITEM(seq, index); + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index + 1); return Py_NewRef(item); } +#ifndef Py_GIL_DISABLED it->it_seq = NULL; Py_DECREF(seq); +#endif return NULL; } diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index e5eb665ae212de..24d80a1ceb63ac 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -24,7 +24,6 @@ race:free_threadstate race_top:assign_version_tag race_top:new_reference race_top:_multiprocessing_SemLock_acquire_impl -race_top:list_get_item_ref race_top:_Py_slot_tp_getattr_hook race_top:add_threadstate race_top:dump_traceback From a069f9b23c1c1a7184fc808615f3752df4a7aeee Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Wed, 8 Jan 2025 16:02:09 +0100 Subject: [PATCH 02/11] Fix lint issues. --- Lib/test/test_free_threading/test_iteration.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_free_threading/test_iteration.py b/Lib/test/test_free_threading/test_iteration.py index 9a88a1e03e36b0..962fc9d1933eaf 100644 --- a/Lib/test/test_free_threading/test_iteration.py +++ b/Lib/test/test_free_threading/test_iteration.py @@ -35,7 +35,7 @@ def run_threads(self, func, *args, numthreads=NUMTHREADS): t.start() threads.append(t) return threads - + def test_iteration(self): """Test iteration over a shared container""" seq = self.make_testdata(NUMITEMS) @@ -53,7 +53,7 @@ def worker(): t.join() # Each thread has its own iterator, so results should be entirely predictable. self.assertEqual(results, [NUMITEMS] * NUMTHREADS) - + def test_shared_iterator(self): """Test iteration over a shared iterator""" seq = self.make_testdata(NUMITEMS) @@ -121,4 +121,3 @@ def make_testdata(self, n): class ContendedLongRangeIterationTest(ContendedTupleIterationTest): def make_testdata(self, n): return range(0, sys.maxsize*n, sys.maxsize) - From 661dc7bf4448fcb1ec6b437bc6559fef608fcab7 Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Thu, 9 Jan 2025 16:10:50 +0100 Subject: [PATCH 03/11] Restore the original guard for list_get_item_ref's fast path, since it's actually correct and the real problem was an incorrect assert. The fast path still contains notionally unsafe uses of memcpy/memmove, so add list_get_item_ref back to the TSan suppressions file. --- Objects/listobject.c | 6 +++--- Tools/tsan/suppressions_free_threading.txt | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 2210ac6e9f68f0..66f30a45e897b6 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -344,7 +344,7 @@ list_item_impl(PyListObject *self, Py_ssize_t idx) static inline PyObject* list_get_item_ref(PyListObject *op, Py_ssize_t i) { - if (!_Py_IsOwnedByCurrentThread((PyObject *)op)) { + if (!_Py_IsOwnedByCurrentThread((PyObject *)op) && !_PyObject_GC_IS_SHARED(op)) { return list_item_impl(op, i); } // Need atomic operation for the getting size. @@ -357,7 +357,7 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) return NULL; } Py_ssize_t cap = list_capacity(ob_item); - assert(cap != -1 && cap >= size); + assert(cap != -1); if (!valid_index(i, cap)) { return NULL; } @@ -937,7 +937,7 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO } for (k = 0; k < n; k++, ilow++) { PyObject *w = vitem[k]; - item[ilow] = Py_XNewRef(w); + FT_ATOMIC_STORE_PTR_RELAXED(item[ilow], Py_XNewRef(w)); } for (k = norig - 1; k >= 0; --k) Py_XDECREF(recycle[k]); diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 24d80a1ceb63ac..968d47490567de 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -29,6 +29,7 @@ race_top:add_threadstate race_top:dump_traceback race_top:fatal_error race_top:_multiprocessing_SemLock_release_impl +race_top:list_get_item_ref race_top:_PyFrame_GetCode race_top:_PyFrame_Initialize race_top:PyInterpreterState_ThreadHead From 484c6b257a64d44f5071a8657756669af47eade9 Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Thu, 9 Jan 2025 17:08:59 +0100 Subject: [PATCH 04/11] Fix test failures in test_sys because of the changed size of range iterators, and fix build failures because labels can't technically be at the end of compound statements (a statement must follow the label, even if it's empty). --- Lib/test/test_sys.py | 6 +++++- Objects/rangeobject.c | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py index d839893d2c657e..eeddd09faed00c 100644 --- a/Lib/test/test_sys.py +++ b/Lib/test/test_sys.py @@ -1689,7 +1689,11 @@ def delx(self): del self.__x # PyCapsule check(_datetime.datetime_CAPI, size('6P')) # rangeiterator - check(iter(range(1)), size('3l')) + if support.Py_GIL_DISABLED: + RANGE_ITER_SIZE = size('4l') + else: + RANGE_ITER_SIZE = size('3l') + check(iter(range(1)), RANGE_ITER_SIZE) check(iter(range(2**65)), size('3P')) # reverse check(reversed(''), size('nP')) diff --git a/Objects/rangeobject.c b/Objects/rangeobject.c index 196838713c2e48..0d2bc3792ef763 100644 --- a/Objects/rangeobject.c +++ b/Objects/rangeobject.c @@ -1033,6 +1033,7 @@ longrangeiter_reduce(longrangeiterobject *r, PyObject *Py_UNUSED(ignored)) result = Py_BuildValue("N(N)O", _PyEval_GetBuiltin(&_Py_ID(iter)), range, Py_None); fail: + ; // A statement must follow the label before Py_END_CRITICAL_SECTION. Py_END_CRITICAL_SECTION(); return result; } @@ -1077,6 +1078,7 @@ longrangeiter_setstate(longrangeiterobject *r, PyObject *state) Py_DECREF(tmp); result = Py_NewRef(Py_None); fail: + ; // A statement must follow the label before Py_END_CRITICAL_SECTION. Py_END_CRITICAL_SECTION(); return result; } @@ -1121,6 +1123,7 @@ longrangeiter_next(longrangeiterobject *r) r->start = new_start; Py_SETREF(r->len, new_len); fail: + ; // A statement must follow the label before Py_END_CRITICAL_SECTION. Py_END_CRITICAL_SECTION(); return result; } From daacf8202a1778073e2cd1b4f4a781a9fa176c41 Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Fri, 10 Jan 2025 13:47:52 +0100 Subject: [PATCH 05/11] Fix tupleiter_reduce for test_iter's assumptions, and make tupleiter_len and tupleiter_setstate appropriately relaxed. --- Objects/tupleobject.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index abf31b567aaef5..a147de27e89a35 100644 --- a/Objects/tupleobject.c +++ b/Objects/tupleobject.c @@ -1039,8 +1039,14 @@ tupleiter_len(PyObject *self, PyObject *Py_UNUSED(ignored)) { _PyTupleIterObject *it = _PyTupleIterObject_CAST(self); Py_ssize_t len = 0; +#ifdef Py_GIL_DISABLED + Py_ssize_t idx = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + if (it->it_seq && idx >= 0 && idx < PyTuple_GET_SIZE(it->it_seq)) + len = PyTuple_GET_SIZE(it->it_seq) - idx; +#else if (it->it_seq) len = PyTuple_GET_SIZE(it->it_seq) - it->it_index; +#endif return PyLong_FromSsize_t(len); } @@ -1056,10 +1062,15 @@ tupleiter_reduce(PyObject *self, PyObject *Py_UNUSED(ignored)) * see issue #101765 */ _PyTupleIterObject *it = _PyTupleIterObject_CAST(self); +#ifdef Py_GIL_DISABLED + Py_ssize_t idx = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + if (it->it_seq && idx >= 0 && idx < PyTuple_GET_SIZE(it->it_seq)) + return Py_BuildValue("N(O)n", iter, it->it_seq, idx); +#else if (it->it_seq) return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index); - else - return Py_BuildValue("N(())", iter); +#endif + return Py_BuildValue("N(())", iter); } static PyObject * @@ -1074,7 +1085,7 @@ tupleiter_setstate(PyObject *self, PyObject *state) index = 0; else if (index > PyTuple_GET_SIZE(it->it_seq)) index = PyTuple_GET_SIZE(it->it_seq); /* exhausted iterator */ - it->it_index = index; + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index); } Py_RETURN_NONE; } From 94ab0c64cc26f4973651b87ce837d19a695989cd Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Sun, 12 Jan 2025 17:05:54 +0100 Subject: [PATCH 06/11] Undo the range iterator changes (leave them for later), adjust the range iterator test to accept the current behaviour of the fast range iterator and avoid testing the (very unsafe) long range iterator, use threading.Barrier where appropriate, and add comments to the unsafe uses of memcpy/memmove in free-threaded builds. --- Include/internal/pycore_range.h | 6 -- Lib/test/test_free_threading/__init__.py | 4 +- .../test_free_threading/test_iteration.py | 31 +++--- Lib/test/test_sys.py | 6 +- Objects/listobject.c | 6 +- Objects/rangeobject.c | 94 ++++++------------- Tools/tsan/suppressions_free_threading.txt | 2 +- 7 files changed, 56 insertions(+), 93 deletions(-) diff --git a/Include/internal/pycore_range.h b/Include/internal/pycore_range.h index 491c7297c9c05b..bf045ec4fd8332 100644 --- a/Include/internal/pycore_range.h +++ b/Include/internal/pycore_range.h @@ -13,12 +13,6 @@ typedef struct { long start; long step; long len; -#ifdef Py_GIL_DISABLED - // Make sure multi-threaded use of a single iterator doesn't produce - // values past the end of the range (even though it is NOT guaranteed to - // uniquely produce all the values in the range!) - long stop; -#endif } _PyRangeIterObject; #ifdef __cplusplus diff --git a/Lib/test/test_free_threading/__init__.py b/Lib/test/test_free_threading/__init__.py index 34e1ad30e11097..fcd5641a149881 100644 --- a/Lib/test/test_free_threading/__init__.py +++ b/Lib/test/test_free_threading/__init__.py @@ -4,8 +4,8 @@ from test import support -if not support.Py_GIL_DISABLED: - raise unittest.SkipTest("GIL enabled") +#if not support.Py_GIL_DISABLED: +# raise unittest.SkipTest("GIL enabled") def load_tests(*args): return support.load_package_tests(os.path.dirname(__file__), *args) diff --git a/Lib/test/test_free_threading/test_iteration.py b/Lib/test/test_free_threading/test_iteration.py index 962fc9d1933eaf..b2c4a898b25475 100644 --- a/Lib/test/test_free_threading/test_iteration.py +++ b/Lib/test/test_free_threading/test_iteration.py @@ -13,7 +13,7 @@ NUMITEMS = 1000 NUMTHREADS = 2 else: - NUMITEMS = 50000 + NUMITEMS = 100000 NUMTHREADS = 3 NUMMUTATORS = 2 @@ -24,7 +24,7 @@ def make_testdata(self, n): def assert_iterator_results(self, results, expected): # Most iterators are not atomic (yet?) so they can skip or duplicate # items, but they should not invent new items (like the range - # iterator has done in the past). + # iterator currently does). extra_items = set(results) - set(expected) self.assertEqual(set(), extra_items) @@ -40,7 +40,7 @@ def test_iteration(self): """Test iteration over a shared container""" seq = self.make_testdata(NUMITEMS) results = [] - start = threading.Event() + start = threading.Barrier(NUMTHREADS) def worker(): idx = 0 start.wait() @@ -48,7 +48,6 @@ def worker(): idx += 1 results.append(idx) threads = self.run_threads(worker) - start.set() for t in threads: t.join() # Each thread has its own iterator, so results should be entirely predictable. @@ -59,7 +58,7 @@ def test_shared_iterator(self): seq = self.make_testdata(NUMITEMS) it = iter(seq) results = [] - start = threading.Event() + start = threading.Barrier(NUMTHREADS) def worker(): items = [] start.wait() @@ -68,10 +67,9 @@ def worker(): items.append(item) results.extend(items) threads = self.run_threads(worker) - start.set() for t in threads: t.join() - self.assert_iterator_results(sorted(results), seq) + self.assert_iterator_results(results, seq) class ContendedListIterationTest(ContendedTupleIterationTest): def make_testdata(self, n): @@ -81,7 +79,7 @@ def test_iteration_while_mutating(self): """Test iteration over a shared mutating container.""" seq = self.make_testdata(NUMITEMS) results = [] - start = threading.Event() + start = threading.Barrier(NUMTHREADS + NUMMUTATORS) endmutate = threading.Event() def mutator(): orig = seq[:] @@ -103,7 +101,6 @@ def worker(): try: threads = self.run_threads(worker) mutators = self.run_threads(mutator, numthreads=NUMMUTATORS) - start.set() for t in threads: t.join() finally: @@ -117,7 +114,17 @@ class ContendedRangeIterationTest(ContendedTupleIterationTest): def make_testdata(self, n): return range(n) + def assert_iterator_results(self, results, expected): + # Range iterators that are shared between threads will (right now) + # sometimes produce items after the end of the range, sometimes + # _far_ after the end of the range. That should be fixed, but for + # now, let's just check they're integers that could have resulted + # from stepping beyond the range bounds. + extra_items = set(results) - set(expected) + for item in extra_items: + self.assertEqual((item - expected.start) % expected.step, 0) -class ContendedLongRangeIterationTest(ContendedTupleIterationTest): - def make_testdata(self, n): - return range(0, sys.maxsize*n, sys.maxsize) +# Long iterators are not thread-safe yet. +# class ContendedLongRangeIterationTest(ContendedTupleIterationTest): +# def make_testdata(self, n): +# return range(0, sys.maxsize*n, sys.maxsize) diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py index eeddd09faed00c..d839893d2c657e 100644 --- a/Lib/test/test_sys.py +++ b/Lib/test/test_sys.py @@ -1689,11 +1689,7 @@ def delx(self): del self.__x # PyCapsule check(_datetime.datetime_CAPI, size('6P')) # rangeiterator - if support.Py_GIL_DISABLED: - RANGE_ITER_SIZE = size('4l') - else: - RANGE_ITER_SIZE = size('3l') - check(iter(range(1)), RANGE_ITER_SIZE) + check(iter(range(1)), size('3l')) check(iter(range(2**65)), size('3P')) # reverse check(reversed(''), size('nP')) diff --git a/Objects/listobject.c b/Objects/listobject.c index 66f30a45e897b6..99c0a860f283b0 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -919,6 +919,8 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO if (d < 0) { /* Delete -d items */ Py_ssize_t tail; tail = (Py_SIZE(a) - ihigh) * sizeof(PyObject *); + // TODO: these memmove/memcpy calls are not safe for shared lists in + // GIL_DISABLED builds. memmove(&item[ihigh+d], &item[ihigh], tail); if (list_resize(a, Py_SIZE(a) + d) < 0) { memmove(&item[ihigh], &item[ihigh+d], tail); @@ -932,12 +934,14 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO if (list_resize(a, k+d) < 0) goto Error; item = a->ob_item; + // TODO: these memmove/memcpy calls are not safe for shared lists in + // GIL_DISABLED builds. memmove(&item[ihigh+d], &item[ihigh], (k - ihigh)*sizeof(PyObject *)); } for (k = 0; k < n; k++, ilow++) { PyObject *w = vitem[k]; - FT_ATOMIC_STORE_PTR_RELAXED(item[ilow], Py_XNewRef(w)); + FT_ATOMIC_STORE_PTR_RELEASE(item[ilow], Py_XNewRef(w)); } for (k = norig - 1; k >= 0; --k) Py_XDECREF(recycle[k]); diff --git a/Objects/rangeobject.c b/Objects/rangeobject.c index 0d2bc3792ef763..2942ab624edf72 100644 --- a/Objects/rangeobject.c +++ b/Objects/rangeobject.c @@ -7,7 +7,6 @@ #include "pycore_modsupport.h" // _PyArg_NoKwnames() #include "pycore_range.h" #include "pycore_tuple.h" // _PyTuple_ITEMS() -#include "pycore_pyatomic_ft_wrappers.h" /* Support objects whose length is > PY_SSIZE_T_MAX. @@ -817,19 +816,10 @@ PyTypeObject PyRange_Type = { static PyObject * rangeiter_next(_PyRangeIterObject *r) { - long len = FT_ATOMIC_LOAD_LONG_RELAXED(r->len); - if (len > 0) { - long result = FT_ATOMIC_LOAD_LONG_RELAXED(r->start); - FT_ATOMIC_STORE_LONG_RELAXED(r->start, result + r->step); - FT_ATOMIC_STORE_LONG_RELAXED(r->len, len - 1); -#ifdef Py_GIL_DISABLED - // Concurrent calls can cause start to be updated by another thread - // after our len check, so double-check that we're not past the end. - if ((r->step > 0 && result >= r->stop) || - (r->step < 0 && result <= r->stop)) { - return NULL; - } -#endif + if (r->len > 0) { + long result = r->start; + r->start = result + r->step; + r->len--; return PyLong_FromLong(result); } return NULL; @@ -838,7 +828,7 @@ rangeiter_next(_PyRangeIterObject *r) static PyObject * rangeiter_len(_PyRangeIterObject *r, PyObject *Py_UNUSED(ignored)) { - return PyLong_FromLong(FT_ATOMIC_LOAD_LONG_RELAXED(r->len)); + return PyLong_FromLong(r->len); } PyDoc_STRVAR(length_hint_doc, @@ -851,14 +841,10 @@ rangeiter_reduce(_PyRangeIterObject *r, PyObject *Py_UNUSED(ignored)) PyObject *range; /* create a range object for pickling */ - start = PyLong_FromLong(FT_ATOMIC_LOAD_LONG_RELAXED(r->start)); + start = PyLong_FromLong(r->start); if (start == NULL) goto err; -#ifdef Py_GIL_DISABLED - stop = PyLong_FromLong(r->stop); -#else stop = PyLong_FromLong(r->start + r->len * r->step); -#endif if (stop == NULL) goto err; step = PyLong_FromLong(r->step); @@ -884,15 +870,13 @@ rangeiter_setstate(_PyRangeIterObject *r, PyObject *state) long index = PyLong_AsLong(state); if (index == -1 && PyErr_Occurred()) return NULL; - long len = FT_ATOMIC_LOAD_LONG_RELAXED(r->len); /* silently clip the index value */ if (index < 0) index = 0; - else if (index > len) - index = len; /* exhausted iterator */ - FT_ATOMIC_STORE_LONG_RELAXED(r->start, - FT_ATOMIC_LOAD_LONG_RELAXED(r->start) + index * r->step); - FT_ATOMIC_STORE_LONG_RELAXED(r->len, len - index); + else if (index > r->len) + index = r->len; /* exhausted iterator */ + r->start += index * r->step; + r->len -= index; Py_RETURN_NONE; } @@ -982,9 +966,6 @@ fast_range_iter(long start, long stop, long step, long len) it->start = start; it->step = step; it->len = len; -#ifdef Py_GIL_DISABLED - it->stop = stop; -#endif return (PyObject *)it; } @@ -998,44 +979,36 @@ typedef struct { static PyObject * longrangeiter_len(longrangeiterobject *r, PyObject *no_args) { - PyObject *len; - Py_BEGIN_CRITICAL_SECTION(r); - len = Py_NewRef(r->len); - Py_END_CRITICAL_SECTION(); - return len; + Py_INCREF(r->len); + return r->len; } static PyObject * longrangeiter_reduce(longrangeiterobject *r, PyObject *Py_UNUSED(ignored)) { PyObject *product, *stop=NULL; - PyObject *range, *result=NULL; + PyObject *range; - Py_BEGIN_CRITICAL_SECTION(r); /* create a range object for pickling. Must calculate the "stop" value */ product = PyNumber_Multiply(r->len, r->step); if (product == NULL) - goto fail; + return NULL; stop = PyNumber_Add(r->start, product); Py_DECREF(product); if (stop == NULL) - goto fail; + return NULL; range = (PyObject*)make_range_object(&PyRange_Type, Py_NewRef(r->start), stop, Py_NewRef(r->step)); if (range == NULL) { Py_DECREF(r->start); Py_DECREF(stop); Py_DECREF(r->step); - goto fail; + return NULL; } /* return the result */ - result = Py_BuildValue("N(N)O", _PyEval_GetBuiltin(&_Py_ID(iter)), - range, Py_None); -fail: - ; // A statement must follow the label before Py_END_CRITICAL_SECTION. - Py_END_CRITICAL_SECTION(); - return result; + return Py_BuildValue("N(N)O", _PyEval_GetBuiltin(&_Py_ID(iter)), + range, Py_None); } static PyObject * @@ -1043,44 +1016,38 @@ longrangeiter_setstate(longrangeiterobject *r, PyObject *state) { PyObject *zero = _PyLong_GetZero(); // borrowed reference int cmp; - PyObject *result = NULL; - Py_BEGIN_CRITICAL_SECTION(r); /* clip the value */ cmp = PyObject_RichCompareBool(state, zero, Py_LT); if (cmp < 0) - goto fail; + return NULL; if (cmp > 0) { state = zero; } else { cmp = PyObject_RichCompareBool(r->len, state, Py_LT); if (cmp < 0) - goto fail; + return NULL; if (cmp > 0) state = r->len; } PyObject *product = PyNumber_Multiply(state, r->step); if (product == NULL) - goto fail; + return NULL; PyObject *new_start = PyNumber_Add(r->start, product); Py_DECREF(product); if (new_start == NULL) - goto fail; + return NULL; PyObject *new_len = PyNumber_Subtract(r->len, state); if (new_len == NULL) { Py_DECREF(new_start); - goto fail; + return NULL; } PyObject *tmp = r->start; r->start = new_start; Py_SETREF(r->len, new_len); Py_DECREF(tmp); - result = Py_NewRef(Py_None); -fail: - ; // A statement must follow the label before Py_END_CRITICAL_SECTION. - Py_END_CRITICAL_SECTION(); - return result; + Py_RETURN_NONE; } static PyMethodDef longrangeiter_methods[] = { @@ -1105,26 +1072,21 @@ longrangeiter_dealloc(longrangeiterobject *r) static PyObject * longrangeiter_next(longrangeiterobject *r) { - PyObject *result = NULL; - Py_BEGIN_CRITICAL_SECTION(r); if (PyObject_RichCompareBool(r->len, _PyLong_GetZero(), Py_GT) != 1) - goto fail; + return NULL; PyObject *new_start = PyNumber_Add(r->start, r->step); if (new_start == NULL) { - goto fail; + return NULL; } PyObject *new_len = PyNumber_Subtract(r->len, _PyLong_GetOne()); if (new_len == NULL) { Py_DECREF(new_start); - goto fail; + return NULL; } - result = r->start; + PyObject *result = r->start; r->start = new_start; Py_SETREF(r->len, new_len); -fail: - ; // A statement must follow the label before Py_END_CRITICAL_SECTION. - Py_END_CRITICAL_SECTION(); return result; } diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 968d47490567de..e5eb665ae212de 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -24,12 +24,12 @@ race:free_threadstate race_top:assign_version_tag race_top:new_reference race_top:_multiprocessing_SemLock_acquire_impl +race_top:list_get_item_ref race_top:_Py_slot_tp_getattr_hook race_top:add_threadstate race_top:dump_traceback race_top:fatal_error race_top:_multiprocessing_SemLock_release_impl -race_top:list_get_item_ref race_top:_PyFrame_GetCode race_top:_PyFrame_Initialize race_top:PyInterpreterState_ThreadHead From c6b9200fcdc8668c54236465d01c8566eb13a686 Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Sun, 12 Jan 2025 17:11:21 +0100 Subject: [PATCH 07/11] Remove edit snafu. --- Lib/test/test_free_threading/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_free_threading/__init__.py b/Lib/test/test_free_threading/__init__.py index fcd5641a149881..34e1ad30e11097 100644 --- a/Lib/test/test_free_threading/__init__.py +++ b/Lib/test/test_free_threading/__init__.py @@ -4,8 +4,8 @@ from test import support -#if not support.Py_GIL_DISABLED: -# raise unittest.SkipTest("GIL enabled") +if not support.Py_GIL_DISABLED: + raise unittest.SkipTest("GIL enabled") def load_tests(*args): return support.load_package_tests(os.path.dirname(__file__), *args) From dd56d893521726593775e69873edbfc141fc63aa Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Mon, 20 Jan 2025 14:52:29 +0100 Subject: [PATCH 08/11] Extend tests a little, add suppressions with links to GH issues. --- Lib/test/test_free_threading/test_iteration.py | 10 ++++++---- Objects/listobject.c | 9 ++++++--- Tools/tsan/suppressions_free_threading.txt | 11 +++++++++-- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_free_threading/test_iteration.py b/Lib/test/test_free_threading/test_iteration.py index b2c4a898b25475..2fa9e3a5a1bf5d 100644 --- a/Lib/test/test_free_threading/test_iteration.py +++ b/Lib/test/test_free_threading/test_iteration.py @@ -1,4 +1,3 @@ -import sys import threading import unittest from test import support @@ -14,7 +13,7 @@ NUMTHREADS = 2 else: NUMITEMS = 100000 - NUMTHREADS = 3 + NUMTHREADS = 5 NUMMUTATORS = 2 class ContendedTupleIterationTest(unittest.TestCase): @@ -88,7 +87,10 @@ def mutator(): replacement = (orig * 3)[NUMITEMS//2:] start.wait() while not endmutate.is_set(): - seq[:] = replacement + seq.extend(replacement) + seq[:0] = orig + seq.__imul__(2) + seq.extend(seq) seq[:] = orig def worker(): items = [] @@ -124,7 +126,7 @@ def assert_iterator_results(self, results, expected): for item in extra_items: self.assertEqual((item - expected.start) % expected.step, 0) -# Long iterators are not thread-safe yet. +# Long range iterators are not thread-safe yet. # class ContendedLongRangeIterationTest(ContendedTupleIterationTest): # def make_testdata(self, n): # return range(0, sys.maxsize*n, sys.maxsize) diff --git a/Objects/listobject.c b/Objects/listobject.c index 99c0a860f283b0..2a7b2e6215aa36 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -784,7 +784,8 @@ list_repeat_lock_held(PyListObject *a, Py_ssize_t n) _Py_RefcntAdd(*src, n); *dest++ = *src++; } - + // TODO: _Py_memory_repeat calls are not safe for shared lists in + // GIL_DISABLED builds. (See issue #129069) _Py_memory_repeat((char *)np->ob_item, sizeof(PyObject *)*output_size, sizeof(PyObject *)*input_size); } @@ -920,7 +921,7 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO Py_ssize_t tail; tail = (Py_SIZE(a) - ihigh) * sizeof(PyObject *); // TODO: these memmove/memcpy calls are not safe for shared lists in - // GIL_DISABLED builds. + // GIL_DISABLED builds. (See issue #129069) memmove(&item[ihigh+d], &item[ihigh], tail); if (list_resize(a, Py_SIZE(a) + d) < 0) { memmove(&item[ihigh], &item[ihigh+d], tail); @@ -935,7 +936,7 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO goto Error; item = a->ob_item; // TODO: these memmove/memcpy calls are not safe for shared lists in - // GIL_DISABLED builds. + // GIL_DISABLED builds. (See issue #129069) memmove(&item[ihigh+d], &item[ihigh], (k - ihigh)*sizeof(PyObject *)); } @@ -1021,6 +1022,8 @@ list_inplace_repeat_lock_held(PyListObject *self, Py_ssize_t n) for (Py_ssize_t j = 0; j < input_size; j++) { _Py_RefcntAdd(items[j], n-1); } + // TODO: _Py_memory_repeat calls are not safe for shared lists in + // GIL_DISABLED builds. (See issue #129069) _Py_memory_repeat((char *)items, sizeof(PyObject *)*output_size, sizeof(PyObject *)*input_size); return 0; diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index e5eb665ae212de..c4ea0b355803e4 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -18,13 +18,11 @@ race:set_allocator_unlocked # https://gist.github.com/swtaarrs/08dfe7883b4c975c31ecb39388987a67 race:free_threadstate - # These warnings trigger directly in a CPython function. race_top:assign_version_tag race_top:new_reference race_top:_multiprocessing_SemLock_acquire_impl -race_top:list_get_item_ref race_top:_Py_slot_tp_getattr_hook race_top:add_threadstate race_top:dump_traceback @@ -46,3 +44,12 @@ race_top:set_default_allocator_unlocked # https://gist.github.com/mpage/6962e8870606cfc960e159b407a0cb40 thread:pthread_create + +# Range iteration is not thread-safe yet (issue #129068) +race_top:rangeiter_next + +# List resizing happens through different paths ending in memcpy or memmove +# (for efficiency), which will probably need to rewritten as explicit loops +# of ptr-sized copies to be thread-safe. (Issue #129069) +race:list_ass_slice_lock_held +race:list_inplace_repeat_lock_held From 4028dfe9b13d22860c81ddb3b885ad1a77bf6274 Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Mon, 20 Jan 2025 15:06:33 +0100 Subject: [PATCH 09/11] Address reviewer comments. --- Objects/tupleobject.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index a147de27e89a35..2f7dd21ac2682f 100644 --- a/Objects/tupleobject.c +++ b/Objects/tupleobject.c @@ -1016,14 +1016,16 @@ tupleiter_next(PyObject *self) assert(it != NULL); seq = it->it_seq; +#ifndef Py_GIL_DISABLED if (seq == NULL) return NULL; +#endif assert(PyTuple_Check(seq)); Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); if (index < PyTuple_GET_SIZE(seq)) { - item = PyTuple_GET_ITEM(seq, index); FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index + 1); + item = PyTuple_GET_ITEM(seq, index); return Py_NewRef(item); } @@ -1041,8 +1043,9 @@ tupleiter_len(PyObject *self, PyObject *Py_UNUSED(ignored)) Py_ssize_t len = 0; #ifdef Py_GIL_DISABLED Py_ssize_t idx = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); - if (it->it_seq && idx >= 0 && idx < PyTuple_GET_SIZE(it->it_seq)) - len = PyTuple_GET_SIZE(it->it_seq) - idx; + Py_ssize_t seq_len = PyTuple_GET_SIZE(it->it_seq); + if (idx < seq_len) + len = seq_len - idx; #else if (it->it_seq) len = PyTuple_GET_SIZE(it->it_seq) - it->it_index; @@ -1064,7 +1067,7 @@ tupleiter_reduce(PyObject *self, PyObject *Py_UNUSED(ignored)) #ifdef Py_GIL_DISABLED Py_ssize_t idx = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); - if (it->it_seq && idx >= 0 && idx < PyTuple_GET_SIZE(it->it_seq)) + if (idx < PyTuple_GET_SIZE(it->it_seq)) return Py_BuildValue("N(O)n", iter, it->it_seq, idx); #else if (it->it_seq) From ef5ca7c029454a41e5a343cf3243f3ca27b1acbc Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Tue, 21 Jan 2025 12:52:05 +0100 Subject: [PATCH 10/11] Turn the last few stores/loads of list iterator/reversed iterator into relaxed atomics. (These are not performance-sensitive parts of the code, and this lets us detect races elsewhere involving the iterators.) --- Objects/listobject.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 2a7b2e6215aa36..69666c2c21c78f 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -4000,7 +4000,7 @@ listiter_setstate(PyObject *self, PyObject *state) index = -1; else if (index > PyList_GET_SIZE(it->it_seq)) index = PyList_GET_SIZE(it->it_seq); /* iterator exhausted */ - it->it_index = index; + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index); } Py_RETURN_NONE; } @@ -4152,7 +4152,7 @@ listreviter_setstate(PyObject *self, PyObject *state) index = -1; else if (index > PyList_GET_SIZE(it->it_seq) - 1) index = PyList_GET_SIZE(it->it_seq) - 1; - it->it_index = index; + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index); } Py_RETURN_NONE; } @@ -4169,18 +4169,19 @@ listiter_reduce_general(void *_it, int forward) * call must be before access of iterator pointers. * see issue #101765 */ - /* the objects are not the same, index is of different types! */ if (forward) { iter = _PyEval_GetBuiltin(&_Py_ID(iter)); _PyListIterObject *it = (_PyListIterObject *)_it; - if (it->it_index >= 0) { - return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index); + Py_ssize_t idx = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + if (idx >= 0) { + return Py_BuildValue("N(O)n", iter, it->it_seq, idx); } } else { iter = _PyEval_GetBuiltin(&_Py_ID(reversed)); listreviterobject *it = (listreviterobject *)_it; - if (it->it_index >= 0) { - return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index); + Py_ssize_t idx = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + if (idx >= 0) { + return Py_BuildValue("N(O)n", iter, it->it_seq, idx); } } /* empty iterator, create an empty list */ From f689a8aa5eff49ba47cb1e42bb9078ff2f54df17 Mon Sep 17 00:00:00 2001 From: Thomas Wouters Date: Sun, 26 Jan 2025 22:40:31 +0100 Subject: [PATCH 11/11] Address reviewer comments. --- Lib/test/test_free_threading/test_iteration.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Lib/test/test_free_threading/test_iteration.py b/Lib/test/test_free_threading/test_iteration.py index 2fa9e3a5a1bf5d..a51ad0cf83a006 100644 --- a/Lib/test/test_free_threading/test_iteration.py +++ b/Lib/test/test_free_threading/test_iteration.py @@ -125,8 +125,3 @@ def assert_iterator_results(self, results, expected): extra_items = set(results) - set(expected) for item in extra_items: self.assertEqual((item - expected.start) % expected.step, 0) - -# Long range iterators are not thread-safe yet. -# class ContendedLongRangeIterationTest(ContendedTupleIterationTest): -# def make_testdata(self, n): -# return range(0, sys.maxsize*n, sys.maxsize)