gh-120608: Make reversed iterator work with free-threading (#120971)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>

diff --git a/Lib/test/test_free_threading/test_reversed.py b/Lib/test/test_free_threading/test_reversed.py
new file mode 100644 (file)
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 2694f5d45c7ebfdfea3dedc0e8f1e75a1b9b63b5..d6a7bd0da599109f6d5741e1ccd6aafd9634b1e3 100644 (file)
--- 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 (file)
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 eb8952675269a23a64d182fb3fa76bcf0d92a17f..17510b29ee70b5d692f674584c062e955631fd0b 100644 (file)
--- 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;
 }