From 1fb7e2aeb7e4312b7f20f0d5f39ddd00d7762004 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Wed, 12 Mar 2025 12:39:52 +0100 Subject: gh-120608: Make reversed iterator work with free-threading (#120971) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Sam Gross Co-authored-by: Kumar Aditya Co-authored-by: Łukasz Langa --- Lib/test/test_free_threading/test_reversed.py | 39 ++++++++++++++++++++++ Lib/test/test_str.py | 3 +- .../2024-06-24-20-08-55.gh-issue-120608.d75n8U.rst | 4 +++ Objects/enumobject.c | 29 +++++++++++----- 4 files changed, 65 insertions(+), 10 deletions(-) create mode 100644 Lib/test/test_free_threading/test_reversed.py create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-06-24-20-08-55.gh-issue-120608.d75n8U.rst diff --git a/Lib/test/test_free_threading/test_reversed.py b/Lib/test/test_free_threading/test_reversed.py new file mode 100644 index 0000000..dd60164 --- /dev/null +++ b/Lib/test/test_free_threading/test_reversed.py @@ -0,0 +1,39 @@ +import unittest +from threading import Barrier, Thread +from test.support import threading_helper + +threading_helper.requires_working_threading(module=True) + +class TestReversed(unittest.TestCase): + + @threading_helper.reap_threads + def test_reversed(self): + # Iterating over the iterator with multiple threads should not + # emit TSAN warnings + number_of_iterations = 10 + number_of_threads = 10 + size = 1_000 + + barrier = Barrier(number_of_threads) + def work(r): + barrier.wait() + while True: + try: + l = r.__length_hint__() + next(r) + except StopIteration: + break + assert 0 <= l <= size + x = tuple(range(size)) + + for _ in range(number_of_iterations): + r = reversed(x) + worker_threads = [] + for _ in range(number_of_threads): + worker_threads.append(Thread(target=work, args=[r])) + with threading_helper.start_threads(worker_threads): + pass + barrier.reset() + +if __name__ == "__main__": + unittest.main() diff --git a/Lib/test/test_str.py b/Lib/test/test_str.py index 2694f5d..d6a7bd0 100644 --- a/Lib/test/test_str.py +++ b/Lib/test/test_str.py @@ -2605,7 +2605,8 @@ class StrTest(string_tests.StringLikeTest, def test_free_after_iterating(self): support.check_free_after_iterating(self, iter, str) - support.check_free_after_iterating(self, reversed, str) + if not support.Py_GIL_DISABLED: + support.check_free_after_iterating(self, reversed, str) def test_check_encoding_errors(self): # bpo-37388: str(bytes) and str.decode() must check encoding and errors diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-06-24-20-08-55.gh-issue-120608.d75n8U.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-06-24-20-08-55.gh-issue-120608.d75n8U.rst new file mode 100644 index 0000000..31d1dfd --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-06-24-20-08-55.gh-issue-120608.d75n8U.rst @@ -0,0 +1,4 @@ +Adapt :func:`reversed` for use in the free-theading build. +The :func:`reversed` is still not thread-safe in the sense that concurrent +iterations may see the same object, but they will not corrupt the interpreter +state. diff --git a/Objects/enumobject.c b/Objects/enumobject.c index eb89526..17510b2 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -439,20 +439,22 @@ reversed_next(PyObject *op) { reversedobject *ro = _reversedobject_CAST(op); PyObject *item; - Py_ssize_t index = ro->index; + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(ro->index); if (index >= 0) { item = PySequence_GetItem(ro->seq, index); if (item != NULL) { - ro->index--; + FT_ATOMIC_STORE_SSIZE_RELAXED(ro->index, index - 1); return item; } if (PyErr_ExceptionMatches(PyExc_IndexError) || PyErr_ExceptionMatches(PyExc_StopIteration)) PyErr_Clear(); } - ro->index = -1; + FT_ATOMIC_STORE_SSIZE_RELAXED(ro->index, -1); +#ifndef Py_GIL_DISABLED Py_CLEAR(ro->seq); +#endif return NULL; } @@ -461,13 +463,15 @@ reversed_len(PyObject *op, PyObject *Py_UNUSED(ignored)) { reversedobject *ro = _reversedobject_CAST(op); Py_ssize_t position, seqsize; + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(ro->index); - if (ro->seq == NULL) + if (index == -1) return PyLong_FromLong(0); + assert(ro->seq != NULL); seqsize = PySequence_Size(ro->seq); if (seqsize == -1) return NULL; - position = ro->index + 1; + position = index + 1; return PyLong_FromSsize_t((seqsize < position) ? 0 : position); } @@ -477,10 +481,13 @@ static PyObject * reversed_reduce(PyObject *op, PyObject *Py_UNUSED(ignored)) { reversedobject *ro = _reversedobject_CAST(op); - if (ro->seq) + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(ro->index); + if (index != -1) { return Py_BuildValue("O(O)n", Py_TYPE(ro), ro->seq, ro->index); - else + } + else { return Py_BuildValue("O(())", Py_TYPE(ro)); + } } static PyObject * @@ -490,7 +497,11 @@ reversed_setstate(PyObject *op, PyObject *state) Py_ssize_t index = PyLong_AsSsize_t(state); if (index == -1 && PyErr_Occurred()) return NULL; - if (ro->seq != 0) { + Py_ssize_t ro_index = FT_ATOMIC_LOAD_SSIZE_RELAXED(ro->index); + // if the iterator is exhausted we do not set the state + // this is for backwards compatibility reasons. in practice this situation + // will not occur, see gh-120971 + if (ro_index != -1) { Py_ssize_t n = PySequence_Size(ro->seq); if (n < 0) return NULL; @@ -498,7 +509,7 @@ reversed_setstate(PyObject *op, PyObject *state) index = -1; else if (index > n-1) index = n-1; - ro->index = index; + FT_ATOMIC_STORE_SSIZE_RELAXED(ro->index, index); } Py_RETURN_NONE; } -- cgit v0.12