From 588a5b638c4a6f07574f5c872b2f256ddc8eac58 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Fri, 30 May 2025 14:38:47 +1200 Subject: [PATCH 1/8] gh-134908: protect ``textiowrapper_iternext`` with critical section ``textiowrapper_iternext`` calls ``_textiowrapper_writeflush`` but does not take the critical section, making it racy in free-threaded builds. --- Lib/test/test_io.py | 28 +++++++++++++++++++ ...-05-30-15-56-19.gh-issue-134908.3a7PxM.rst | 1 + Modules/_io/textio.c | 14 +++++++++- 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 168e66c5a3f0e0..a6e0c8fc6fc551 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1062,6 +1062,34 @@ def flush(self): # Silence destructor error R.flush = lambda self: None + @threading_helper.requires_working_threading() + def test_write_readline_races(self): + N=2 + COUNT=100 + + def writer(file, barrier): + barrier.wait() + for _ in range(COUNT): + f.write("x") + + def reader(file, stopping): + while not stopping.is_set(): + for line in file: + assert line == "" + + stopping = threading.Event() + with self.open(os_helper.TESTFN, "w+") as f: + barrier = threading.Barrier(N) + reader = threading.Thread(target=reader, args=(f, stopping)) + writers = [threading.Thread(target=writer, args=(f, barrier)) + for _ in range(N)] + reader.start() + with threading_helper.start_threads(writers): + pass + stopping.set() + reader.join() + assert(os.stat(os_helper.TESTFN).st_size == COUNT * N) + class CIOTest(IOTest): diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst new file mode 100644 index 00000000000000..8bdcf3a705d5aa --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst @@ -0,0 +1 @@ +Protect ``textiowrapper_iternext`` with a critical section. diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index 86328e46a7b131..89932d2280721e 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -1578,6 +1578,8 @@ _io_TextIOWrapper_detach_impl(textio *self) static int _textiowrapper_writeflush(textio *self) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); + if (self->pending_bytes == NULL) return 0; @@ -3173,7 +3175,7 @@ _io_TextIOWrapper_close_impl(textio *self) } static PyObject * -textiowrapper_iternext(PyObject *op) +textiowrapper_iternext_locked(PyObject *op) { PyObject *line; textio *self = textio_CAST(op); @@ -3210,6 +3212,16 @@ textiowrapper_iternext(PyObject *op) return line; } +static PyObject * +textiowrapper_iternext(PyObject *op) +{ + PyObject *result; + Py_BEGIN_CRITICAL_SECTION(op); + result = textiowrapper_iternext_locked(op); + Py_END_CRITICAL_SECTION(); + return result; +} + /*[clinic input] @critical_section @getter From 25e55724f2e02a9ba97fb6ac1d5305cf6c1c5cc8 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Fri, 30 May 2025 18:57:18 +1200 Subject: [PATCH 2/8] Address review feedback (thanks!) --- Lib/test/test_io.py | 16 +++++++++------- ...025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index a6e0c8fc6fc551..f7aa0bb58f9fde 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1064,13 +1064,13 @@ def flush(self): @threading_helper.requires_working_threading() def test_write_readline_races(self): - N=2 - COUNT=100 + thread_count = 2 + write_count = 100 def writer(file, barrier): barrier.wait() - for _ in range(COUNT): - f.write("x") + for _ in range(write_count): + file.write("x") def reader(file, stopping): while not stopping.is_set(): @@ -1079,16 +1079,18 @@ def reader(file, stopping): stopping = threading.Event() with self.open(os_helper.TESTFN, "w+") as f: - barrier = threading.Barrier(N) + barrier = threading.Barrier(thread_count) reader = threading.Thread(target=reader, args=(f, stopping)) writers = [threading.Thread(target=writer, args=(f, barrier)) - for _ in range(N)] + for _ in range(thread_count)] reader.start() with threading_helper.start_threads(writers): pass stopping.set() reader.join() - assert(os.stat(os_helper.TESTFN).st_size == COUNT * N) + + self.assertEqual(os.stat(os_helper.TESTFN).st_size, + write_count * thread_count) class CIOTest(IOTest): diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst index 8bdcf3a705d5aa..b6ac179adbb162 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst @@ -1 +1 @@ -Protect ``textiowrapper_iternext`` with a critical section. +Make iterating over lines in a text file thread safe in free threading. From 84e853712e6b0bb9db501d510928d059cde63fcd Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Fri, 30 May 2025 23:04:49 +1200 Subject: [PATCH 3/8] Update Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst Co-authored-by: Peter Bierma --- .../2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst index b6ac179adbb162..4c0207d098256e 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst @@ -1 +1 @@ -Make iterating over lines in a text file thread safe in free threading. +Fix crash when iterating over lines in a text file thread safe on the :term:`free threaded ` build. From aebcc9880f9e493f22faef8a0a8e0450b7362d1f Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 1 Jun 2025 14:35:01 +1200 Subject: [PATCH 4/8] Update Modules/_io/textio.c Co-authored-by: Peter Bierma --- Modules/_io/textio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index 89932d2280721e..211f37782f3e4f 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -3217,7 +3217,7 @@ textiowrapper_iternext(PyObject *op) { PyObject *result; Py_BEGIN_CRITICAL_SECTION(op); - result = textiowrapper_iternext_locked(op); + result = textiowrapper_iternext_lock_held(op); Py_END_CRITICAL_SECTION(); return result; } From 72d1f9d6ba6e0ac301573aee54b169d9fac5bdaf Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 1 Jun 2025 14:35:07 +1200 Subject: [PATCH 5/8] Update Modules/_io/textio.c Co-authored-by: Peter Bierma --- Modules/_io/textio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index 211f37782f3e4f..3808ecdceb9b70 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -3175,8 +3175,9 @@ _io_TextIOWrapper_close_impl(textio *self) } static PyObject * -textiowrapper_iternext_locked(PyObject *op) +textiowrapper_iternext_lock_held(PyObject *op) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op); PyObject *line; textio *self = textio_CAST(op); From ad17efb2d5ae8c80c3877398e0d7a417409c826d Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 1 Jun 2025 14:39:57 +1200 Subject: [PATCH 6/8] More improvements after review feedback --- Lib/test/test_io.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index f7aa0bb58f9fde..339cffc2f4cc85 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1064,6 +1064,8 @@ def flush(self): @threading_helper.requires_working_threading() def test_write_readline_races(self): + + # gh-134908: Concurrent iteration over a file caused races thread_count = 2 write_count = 100 @@ -1075,7 +1077,7 @@ def writer(file, barrier): def reader(file, stopping): while not stopping.is_set(): for line in file: - assert line == "" + self.assertEqual(line, "") stopping = threading.Event() with self.open(os_helper.TESTFN, "w+") as f: @@ -1083,11 +1085,13 @@ def reader(file, stopping): reader = threading.Thread(target=reader, args=(f, stopping)) writers = [threading.Thread(target=writer, args=(f, barrier)) for _ in range(thread_count)] - reader.start() - with threading_helper.start_threads(writers): - pass - stopping.set() - reader.join() + with threading_helper.catch_threading_exception() as cm: + reader.start() + with threading_helper.start_threads(writers): + pass + stopping.set() + reader.join() + self.assertIsNone(cm.exc_type) self.assertEqual(os.stat(os_helper.TESTFN).st_size, write_count * thread_count) From ee6ef4488cfdc06b22e1e533ffab6a7ff6eb8f3a Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 2 Jun 2025 12:12:57 -0400 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Peter Bierma --- Lib/test/test_io.py | 1 - .../2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 339cffc2f4cc85..afa97cabd9223f 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1064,7 +1064,6 @@ def flush(self): @threading_helper.requires_working_threading() def test_write_readline_races(self): - # gh-134908: Concurrent iteration over a file caused races thread_count = 2 write_count = 100 diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst index 4c0207d098256e..3178f0aaf885f8 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-30-15-56-19.gh-issue-134908.3a7PxM.rst @@ -1 +1 @@ -Fix crash when iterating over lines in a text file thread safe on the :term:`free threaded ` build. +Fix crash when iterating over lines in a text file on the :term:`free threaded ` build. From c15ea28306589b5b81394508bf5de600201f6335 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 2 Jun 2025 16:13:21 +0000 Subject: [PATCH 8/8] Make test a bit faster on the GIL-enabled build --- Lib/test/test_io.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index afa97cabd9223f..0c921ffbc2576a 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1067,29 +1067,27 @@ def test_write_readline_races(self): # gh-134908: Concurrent iteration over a file caused races thread_count = 2 write_count = 100 + read_count = 100 def writer(file, barrier): barrier.wait() for _ in range(write_count): file.write("x") - def reader(file, stopping): - while not stopping.is_set(): + def reader(file, barrier): + barrier.wait() + for _ in range(read_count): for line in file: self.assertEqual(line, "") - stopping = threading.Event() with self.open(os_helper.TESTFN, "w+") as f: - barrier = threading.Barrier(thread_count) - reader = threading.Thread(target=reader, args=(f, stopping)) + barrier = threading.Barrier(thread_count + 1) + reader = threading.Thread(target=reader, args=(f, barrier)) writers = [threading.Thread(target=writer, args=(f, barrier)) for _ in range(thread_count)] with threading_helper.catch_threading_exception() as cm: - reader.start() - with threading_helper.start_threads(writers): + with threading_helper.start_threads(writers + [reader]): pass - stopping.set() - reader.join() self.assertIsNone(cm.exc_type) self.assertEqual(os.stat(os_helper.TESTFN).st_size,