From 1e2330c0e13a49dc055049b29866a1ffb7bcd13f Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 24 Apr 2024 13:13:13 -0700 Subject: [PATCH 1/5] Fix TSAN list set failure --- Include/cpython/listobject.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Include/cpython/listobject.h b/Include/cpython/listobject.h index 49f5e8d6d1a0d6..febed776af748d 100644 --- a/Include/cpython/listobject.h +++ b/Include/cpython/listobject.h @@ -44,7 +44,11 @@ PyList_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) { PyListObject *list = _PyList_CAST(op); assert(0 <= index); assert(index < list->allocated); +#ifdef Py_GIL_DISABLED + _Py_atomic_store_ptr_release(&list->ob_item[index], value); +#else list->ob_item[index] = value; +#endif } #define PyList_SET_ITEM(op, index, value) \ PyList_SET_ITEM(_PyObject_CAST(op), (index), _PyObject_CAST(value)) From 9d3838caf8b31128ac3f01f32b7be6a3dc03f584 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Thu, 25 Apr 2024 12:28:12 -0700 Subject: [PATCH 2/5] Relaxed atomic is sufficient, add targetted test --- Include/cpython/listobject.h | 2 +- Lib/test/test_free_threading/test_list.py | 38 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 Lib/test/test_free_threading/test_list.py diff --git a/Include/cpython/listobject.h b/Include/cpython/listobject.h index febed776af748d..85150b168e0800 100644 --- a/Include/cpython/listobject.h +++ b/Include/cpython/listobject.h @@ -45,7 +45,7 @@ PyList_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) { assert(0 <= index); assert(index < list->allocated); #ifdef Py_GIL_DISABLED - _Py_atomic_store_ptr_release(&list->ob_item[index], value); + _Py_atomic_store_ptr_relaxed(&list->ob_item[index], value); #else list->ob_item[index] = value; #endif diff --git a/Lib/test/test_free_threading/test_list.py b/Lib/test/test_free_threading/test_list.py new file mode 100644 index 00000000000000..a1d884fe02b84f --- /dev/null +++ b/Lib/test/test_free_threading/test_list.py @@ -0,0 +1,38 @@ +import unittest + +from threading import Thread +from unittest import TestCase + +class TestList(TestCase): + def test_racing_iter_append(self): + + l = [] + OBJECT_COUNT = 10000 + def writer_func(): + for i in range(OBJECT_COUNT): + l.append(i + OBJECT_COUNT) + + def reader_func(): + while True: + count = len(l) + for i, x in enumerate(l): + self.assertEqual(x, i + OBJECT_COUNT) + if count == OBJECT_COUNT: + break + + writer = Thread(target=writer_func) + readers = [] + for x in range(30): + reader = Thread(target=reader_func) + readers.append(reader) + reader.start() + + writer.start() + writer.join() + print('writer done') + for reader in readers: + reader.join() + print('reader done') + +if __name__ == "__main__": + unittest.main() From 4b8aed8899b802b1ff4abae8d0d23b4a064d4e4d Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Mon, 29 Apr 2024 11:52:10 -0700 Subject: [PATCH 3/5] More list --- Include/internal/pycore_list.h | 4 ++ Lib/test/test_free_threading/test_list.py | 50 +++++++++++++++++++++-- Objects/listobject.c | 10 +++-- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/Include/internal/pycore_list.h b/Include/internal/pycore_list.h index 2a82912e41d557..73695d10e0c372 100644 --- a/Include/internal/pycore_list.h +++ b/Include/internal/pycore_list.h @@ -28,7 +28,11 @@ _PyList_AppendTakeRef(PyListObject *self, PyObject *newitem) Py_ssize_t allocated = self->allocated; assert((size_t)len + 1 < PY_SSIZE_T_MAX); if (allocated > len) { +#ifdef Py_GIL_DISABLED + _Py_atomic_store_ptr_release(&self->ob_item[len], newitem); +#else PyList_SET_ITEM(self, len, newitem); +#endif Py_SET_SIZE(self, len + 1); return 0; } diff --git a/Lib/test/test_free_threading/test_list.py b/Lib/test/test_free_threading/test_list.py index a1d884fe02b84f..79cb0a93092365 100644 --- a/Lib/test/test_free_threading/test_list.py +++ b/Lib/test/test_free_threading/test_list.py @@ -3,20 +3,30 @@ from threading import Thread from unittest import TestCase +from test.support import is_wasi + + +class C: + def __init__(self, v): + self.v = v + + +@unittest.skipIf(is_wasi, "WASI has no threads.") class TestList(TestCase): def test_racing_iter_append(self): l = [] OBJECT_COUNT = 10000 + def writer_func(): for i in range(OBJECT_COUNT): - l.append(i + OBJECT_COUNT) + l.append(C(i + OBJECT_COUNT)) def reader_func(): while True: count = len(l) for i, x in enumerate(l): - self.assertEqual(x, i + OBJECT_COUNT) + self.assertEqual(x.v, i + OBJECT_COUNT) if count == OBJECT_COUNT: break @@ -29,10 +39,42 @@ def reader_func(): writer.start() writer.join() - print('writer done') for reader in readers: reader.join() - print('reader done') + + def test_racing_iter_extend(self): + iters = [ + lambda x: [x], + ] + for iter_case in iters: + with self.subTest(iter=iter_case): + l = [] + OBJECT_COUNT = 10000 + + def writer_func(): + for i in range(OBJECT_COUNT): + l.extend(iter_case(C(i + OBJECT_COUNT))) + + def reader_func(): + while True: + count = len(l) + for i, x in enumerate(l): + self.assertEqual(x.v, i + OBJECT_COUNT) + if count == OBJECT_COUNT: + break + + writer = Thread(target=writer_func) + readers = [] + for x in range(30): + reader = Thread(target=reader_func) + readers.append(reader) + reader.start() + + writer.start() + writer.join() + for reader in readers: + reader.join() + if __name__ == "__main__": unittest.main() diff --git a/Objects/listobject.c b/Objects/listobject.c index 4eaf20033fa262..85c52b6f1e6c7a 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -45,6 +45,7 @@ list_allocate_array(size_t capacity) if (capacity > PY_SSIZE_T_MAX/sizeof(PyObject*) - 1) { return NULL; } + _PyListArray *array = PyMem_Malloc(sizeof(_PyListArray) + capacity * sizeof(PyObject *)); if (array == NULL) { return NULL; @@ -141,6 +142,9 @@ list_resize(PyListObject *self, Py_ssize_t newsize) target_bytes = allocated * sizeof(PyObject*); } memcpy(array->ob_item, self->ob_item, target_bytes); + } + if (new_allocated > (size_t)allocated) { + memset(array->ob_item + allocated, 0, sizeof(PyObject *) * (new_allocated - allocated)); } _Py_atomic_store_ptr_release(&self->ob_item, &array->ob_item); self->allocated = new_allocated; @@ -502,7 +506,7 @@ _PyList_AppendTakeRefListResize(PyListObject *self, PyObject *newitem) Py_DECREF(newitem); return -1; } - PyList_SET_ITEM(self, len, newitem); + FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], newitem); return 0; } @@ -1181,7 +1185,7 @@ list_extend_fast(PyListObject *self, PyObject *iterable) PyObject **dest = self->ob_item + m; for (Py_ssize_t i = 0; i < n; i++) { PyObject *o = src[i]; - dest[i] = Py_NewRef(o); + FT_ATOMIC_STORE_PTR_RELEASE(dest[i], Py_NewRef(o)); } return 0; } @@ -1238,7 +1242,7 @@ list_extend_iter_lock_held(PyListObject *self, PyObject *iterable) if (Py_SIZE(self) < self->allocated) { Py_ssize_t len = Py_SIZE(self); - PyList_SET_ITEM(self, len, item); // steals item ref + FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], item); Py_SET_SIZE(self, len + 1); } else { From 0da608b4a65dd8ed0bdda8054eeaf0c851d956ed Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Tue, 30 Apr 2024 11:37:13 -0700 Subject: [PATCH 4/5] Remove atomic assign in list --- Include/cpython/listobject.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Include/cpython/listobject.h b/Include/cpython/listobject.h index 85150b168e0800..49f5e8d6d1a0d6 100644 --- a/Include/cpython/listobject.h +++ b/Include/cpython/listobject.h @@ -44,11 +44,7 @@ PyList_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) { PyListObject *list = _PyList_CAST(op); assert(0 <= index); assert(index < list->allocated); -#ifdef Py_GIL_DISABLED - _Py_atomic_store_ptr_relaxed(&list->ob_item[index], value); -#else list->ob_item[index] = value; -#endif } #define PyList_SET_ITEM(op, index, value) \ PyList_SET_ITEM(_PyObject_CAST(op), (index), _PyObject_CAST(value)) From 4ad28e6afba57f802569602125cab4de36139df4 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 1 May 2024 11:34:15 -0700 Subject: [PATCH 5/5] Fixup white space --- Objects/listobject.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 85c52b6f1e6c7a..3c4e2d2e6ed7de 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -45,7 +45,6 @@ list_allocate_array(size_t capacity) if (capacity > PY_SSIZE_T_MAX/sizeof(PyObject*) - 1) { return NULL; } - _PyListArray *array = PyMem_Malloc(sizeof(_PyListArray) + capacity * sizeof(PyObject *)); if (array == NULL) { return NULL; @@ -1242,7 +1241,7 @@ list_extend_iter_lock_held(PyListObject *self, PyObject *iterable) if (Py_SIZE(self) < self->allocated) { Py_ssize_t len = Py_SIZE(self); - FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], item); + FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], item); // steals item ref Py_SET_SIZE(self, len + 1); } else {