gh-129107: make bytearrayiter free-threading safe

Author: tom-pytel

Lib/test/test_bytes.py

@@ -11,12 +11,15 @@
 import copy
 import functools
 import pickle
+import sysconfig
 import tempfile
 import textwrap
+import threading
 import unittest
 
 import test.support
 from test.support import import_helper
+from test.support import threading_helper
 from test.support import warnings_helper
 import test.string_tests
 import test.list_tests
@@ -2185,5 +2188,39 @@ class BytesSubclassTest(SubclassTest, unittest.TestCase):
     type2test = BytesSubclass
 
 
+class FreeThreadingTest(unittest.TestCase):
+    @unittest.skipUnless(sysconfig.get_config_var('Py_GIL_DISABLED'),
+                         'this test can only possibly fail with GIL disabled')
+    @threading_helper.reap_threads
+    @threading_helper.requires_working_threading()
+    def test_free_threading_bytearrayiter(self):
+        # Non-deterministic but good chance to fail if bytearrayiter is not free-threading safe.
+        # We are fishing for a "Assertion failed: object has negative ref count".
+
+        def iter_next(b, it):
+            b.wait()
+            list(it)
+
+        def check(funcs, it):
+            barrier = threading.Barrier(len(funcs))
+            threads = []
+
+            for func in funcs:
+                thread = threading.Thread(target=func, args=(barrier, it))
+
+                threads.append(thread)
+
+            with threading_helper.start_threads(threads):
+                pass
+
+            for thread in threads:
+                threading_helper.join_thread(thread)
+
+        for _ in range(10):
+            ba = bytearray(b'0' * 0x4000)  # this is a load-bearing variable, do not remove
+
+            check([iter_next] * 10, iter(ba))
+
+
 if __name__ == "__main__":
     unittest.main()

Objects/bytearrayobject.c

@@ -5,6 +5,7 @@
 #include "pycore_bytes_methods.h"
 #include "pycore_bytesobject.h"
 #include "pycore_ceval.h"         // _PyEval_GetBuiltin()
+#include "pycore_critical_section.h"
 #include "pycore_object.h"        // _PyObject_GC_UNTRACK()
 #include "pycore_strhex.h"        // _Py_strhex_with_sep()
 #include "pycore_long.h"          // _PyLong_FromUnsignedChar()
@@ -2519,31 +2520,45 @@ static PyObject *
 bytearrayiter_next(PyObject *self)
 {
     bytesiterobject *it = _bytesiterobject_CAST(self);
-    PyByteArrayObject *seq;
+    int val;
 
     assert(it != NULL);
-    seq = it->it_seq;
-    if (seq == NULL)
+    Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index);
+    if (index < 0) {
         return NULL;
+    }
+    PyByteArrayObject *seq = it->it_seq;
     assert(PyByteArray_Check(seq));
 
-    if (it->it_index < PyByteArray_GET_SIZE(seq)) {
-        return _PyLong_FromUnsignedChar(
-            (unsigned char)PyByteArray_AS_STRING(seq)[it->it_index++]);
+    Py_BEGIN_CRITICAL_SECTION(seq);
+    if (index < PyByteArray_GET_SIZE(seq)) {
+        val = (unsigned char)PyByteArray_AS_STRING(seq)[index];
     }
+    else {
+        val = -1;
+    }
+    Py_END_CRITICAL_SECTION();
 
-    it->it_seq = NULL;
-    Py_DECREF(seq);
-    return NULL;
+    if (val == -1) {
+        FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, -1);
+#ifndef Py_GIL_DISABLED
+        it->it_seq = NULL;
+        Py_DECREF(seq);
+#endif
+        return NULL;
+    }
+    FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index + 1);
+    return _PyLong_FromUnsignedChar((unsigned char)val);
 }
 
 static PyObject *
 bytearrayiter_length_hint(PyObject *self, PyObject *Py_UNUSED(ignored))
 {
     bytesiterobject *it = _bytesiterobject_CAST(self);
     Py_ssize_t len = 0;
-    if (it->it_seq) {
-        len = PyByteArray_GET_SIZE(it->it_seq) - it->it_index;
+    Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index);
+    if (index >= 0) {
+        len = PyByteArray_GET_SIZE(it->it_seq) - index;
         if (len < 0) {
             len = 0;
         }
@@ -2563,27 +2578,41 @@ bytearrayiter_reduce(PyObject *self, PyObject *Py_UNUSED(ignored))
      * call must be before access of iterator pointers.
      * see issue #101765 */
     bytesiterobject *it = _bytesiterobject_CAST(self);
-    if (it->it_seq != NULL) {
-        return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
-    } else {
-        return Py_BuildValue("N(())", iter);
+    PyObject *ret = NULL;
+    Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index);
+    if (index >= 0) {
+        Py_BEGIN_CRITICAL_SECTION(it->it_seq);
+        if (index <= PyByteArray_GET_SIZE(it->it_seq)) {
+            ret = Py_BuildValue("N(O)n", iter, it->it_seq, index);
+        }
+        Py_END_CRITICAL_SECTION();
+    }
+    if (ret == NULL) {
+        ret = Py_BuildValue("N(())", iter);
     }
+    return ret;
 }
 
 static PyObject *
 bytearrayiter_setstate(PyObject *self, PyObject *state)
 {
     Py_ssize_t index = PyLong_AsSsize_t(state);
-    if (index == -1 && PyErr_Occurred())
+    if (index == -1 && PyErr_Occurred()) {
         return NULL;
+    }
 
     bytesiterobject *it = _bytesiterobject_CAST(self);
-    if (it->it_seq != NULL) {
-        if (index < 0)
-            index = 0;
-        else if (index > PyByteArray_GET_SIZE(it->it_seq))
-            index = PyByteArray_GET_SIZE(it->it_seq); /* iterator exhausted */
-        it->it_index = index;
+    if (FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index) >= 0) {
+        if (index < -1) {
+            index = -1;
+        }
+        else {
+            Py_ssize_t size = PyByteArray_GET_SIZE(it->it_seq);
+            if (index > size) {
+                index = size; /* iterator at end */
+            }
+        }
+        FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index);
     }
     Py_RETURN_NONE;
 }
@@ -2645,7 +2674,7 @@ bytearray_iter(PyObject *seq)
     it = PyObject_GC_New(bytesiterobject, &PyByteArrayIter_Type);
     if (it == NULL)
         return NULL;
-    it->it_index = 0;
+    it->it_index = 0;  // -1 indicates exhausted
     it->it_seq = (PyByteArrayObject *)Py_NewRef(seq);
     _PyObject_GC_TRACK(it);
     return (PyObject *)it;