From 609ce86a27056d6cf537e9fbde9ce90b0d5c0b6f Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 19 Oct 2024 22:07:02 +0200 Subject: [PATCH 01/16] Make concurrent iteration over enumerate safe under free-threading --- .../test_free_threading/test_enumerate.py | 41 +++++++++++++++++++ Objects/enumobject.c | 15 ++++--- 2 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 Lib/test/test_free_threading/test_enumerate.py diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py new file mode 100644 index 00000000000000..33d282e598d96f --- /dev/null +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -0,0 +1,41 @@ +import unittest +import sys +from threading import Thread + +from test.support import threading_helper + + +class EnumerateThreading(unittest.TestCase): + @staticmethod + def work(enum, start): + while True: + try: + value = next(enum) + except StopIteration: + break + else: + if value[0] + start != value[1]: + raise ValueError(f'enumerate returned pair {value}') + + @threading_helper.reap_threads + @threading_helper.requires_working_threading() + def test_threading(self): + number_of_threads = 4 + number_of_iterations = 8 + n = 100 + start = sys.maxsize - 40 + + for _ in range(number_of_iterations): + enum = enumerate(range(start, start + n)) + worker_threads = [] + for ii in range(number_of_threads): + worker_threads.append( + Thread(target=self.work, args=[enum, start])) + for t in worker_threads: + t.start() + for t in worker_threads: + t.join() + + +if __name__ == "__main__": + unittest.main() diff --git a/Objects/enumobject.c b/Objects/enumobject.c index 556666779d8522..39afb2d826b694 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -177,6 +177,7 @@ enum_next_long(enumobject *en, PyObject* next_item) PyObject *old_index; PyObject *old_item; + Py_BEGIN_CRITICAL_SECTION(en); if (en->en_longindex == NULL) { en->en_longindex = PyLong_FromSsize_t(PY_SSIZE_T_MAX); if (en->en_longindex == NULL) { @@ -192,8 +193,9 @@ enum_next_long(enumobject *en, PyObject* next_item) return NULL; } en->en_longindex = stepped_up; + Py_END_CRITICAL_SECTION(); - if (Py_REFCNT(result) == 1) { + if (_PyObject_IsUniquelyReferenced(result)) { Py_INCREF(result); old_index = PyTuple_GET_ITEM(result, 0); old_item = PyTuple_GET_ITEM(result, 1); @@ -233,17 +235,18 @@ enum_next(enumobject *en) if (next_item == NULL) return NULL; - if (en->en_index == PY_SSIZE_T_MAX) + Py_ssize_t en_index = FT_ATOMIC_LOAD_SSIZE_RELAXED(en->en_index); + if (en_index == PY_SSIZE_T_MAX) return enum_next_long(en, next_item); - next_index = PyLong_FromSsize_t(en->en_index); + next_index = PyLong_FromSsize_t(en_index); if (next_index == NULL) { Py_DECREF(next_item); return NULL; } - en->en_index++; + FT_ATOMIC_STORE_SSIZE_RELAXED(en->en_index, en_index + 1); - if (Py_REFCNT(result) == 1) { + if (_PyObject_IsUniquelyReferenced(result)) { Py_INCREF(result); old_index = PyTuple_GET_ITEM(result, 0); old_item = PyTuple_GET_ITEM(result, 1); @@ -272,10 +275,12 @@ enum_next(enumobject *en) static PyObject * enum_reduce(enumobject *en, PyObject *Py_UNUSED(ignored)) { + Py_BEGIN_CRITICAL_SECTION(en); if (en->en_longindex != NULL) return Py_BuildValue("O(OO)", Py_TYPE(en), en->en_sit, en->en_longindex); else return Py_BuildValue("O(On)", Py_TYPE(en), en->en_sit, en->en_index); + Py_END_CRITICAL_SECTION(); } PyDoc_STRVAR(reduce_doc, "Return state information for pickling."); From 7cbc12ac578f5cc02aac65bbb5838aab4b99b43b Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 19 Oct 2024 20:22:23 +0000 Subject: [PATCH 02/16] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst new file mode 100644 index 00000000000000..8e538add0b5beb --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst @@ -0,0 +1 @@ +Make concurrent iterations over the same :func:`enumerate` iterator safe under free-threading. See [Strategy for Iterators in Free Threading](https://github.com/python/cpython/issues/124397) From 3902f3bec37db9562cad0f95cedf21b407ff93a4 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 19 Oct 2024 22:27:56 +0200 Subject: [PATCH 03/16] fix news entry --- ....IHwfpK.rst => 2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Misc/NEWS.d/next/Core_and_Builtins/{2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst => 2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst} (100%) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst similarity index 100% rename from Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-gh-121464.IHwfpK.rst rename to Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst From ce2315c80f2b910eb4abd72cbba819cebee9dd94 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 19 Oct 2024 22:46:29 +0200 Subject: [PATCH 04/16] relax test --- Lib/test/test_free_threading/test_enumerate.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index 33d282e598d96f..5e742b77db8e7a 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -14,8 +14,7 @@ def work(enum, start): except StopIteration: break else: - if value[0] + start != value[1]: - raise ValueError(f'enumerate returned pair {value}') + assert isinstance(value, tuple) @threading_helper.reap_threads @threading_helper.requires_working_threading() From e42f74f9b0f5baf78fbb0f93b2fde1922aeaad6c Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 19 Oct 2024 22:46:53 +0200 Subject: [PATCH 05/16] relax test --- Lib/test/test_free_threading/test_enumerate.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index 5e742b77db8e7a..02aa096ae0fda5 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -13,8 +13,7 @@ def work(enum, start): value = next(enum) except StopIteration: break - else: - assert isinstance(value, tuple) + @threading_helper.reap_threads @threading_helper.requires_working_threading() From 079e6ab8c727e9bb3e5e5fb367035e1041007144 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sun, 20 Oct 2024 20:51:09 +0200 Subject: [PATCH 06/16] fix exit of critical section --- Objects/enumobject.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Objects/enumobject.c b/Objects/enumobject.c index 39afb2d826b694..83cbcacbac36cf 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -275,12 +275,14 @@ enum_next(enumobject *en) static PyObject * enum_reduce(enumobject *en, PyObject *Py_UNUSED(ignored)) { + PyObject * result; Py_BEGIN_CRITICAL_SECTION(en); if (en->en_longindex != NULL) - return Py_BuildValue("O(OO)", Py_TYPE(en), en->en_sit, en->en_longindex); + result = Py_BuildValue("O(OO)", Py_TYPE(en), en->en_sit, en->en_longindex); else - return Py_BuildValue("O(On)", Py_TYPE(en), en->en_sit, en->en_index); + result = Py_BuildValue("O(On)", Py_TYPE(en), en->en_sit, en->en_index); Py_END_CRITICAL_SECTION(); + return result; } PyDoc_STRVAR(reduce_doc, "Return state information for pickling."); From 0dd6cd821210bedfa21b839917ebb1eeb457e656 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sun, 20 Oct 2024 21:22:48 +0200 Subject: [PATCH 07/16] format news entry --- .../2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst index 8e538add0b5beb..6bf031c9cd7904 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-19-20-22-19.gh-issue-121464.IHwfpK.rst @@ -1 +1 @@ -Make concurrent iterations over the same :func:`enumerate` iterator safe under free-threading. See [Strategy for Iterators in Free Threading](https://github.com/python/cpython/issues/124397) +Make concurrent iterations over the same :func:`enumerate` iterator safe under free-threading. See `Strategy for Iterators in Free Threading `_. From d861c35c969a948834859cf66cf1dd74cfa7ec3c Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Mon, 21 Oct 2024 09:21:35 +0200 Subject: [PATCH 08/16] apply review suggestions --- Lib/test/test_free_threading/test_enumerate.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index 02aa096ae0fda5..bcc6588d34fca5 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -6,14 +6,6 @@ class EnumerateThreading(unittest.TestCase): - @staticmethod - def work(enum, start): - while True: - try: - value = next(enum) - except StopIteration: - break - @threading_helper.reap_threads @threading_helper.requires_working_threading() @@ -23,12 +15,19 @@ def test_threading(self): n = 100 start = sys.maxsize - 40 + def work(enum, start): + while True: + try: + _ = next(enum) + except StopIteration: + break + for _ in range(number_of_iterations): enum = enumerate(range(start, start + n)) worker_threads = [] for ii in range(number_of_threads): worker_threads.append( - Thread(target=self.work, args=[enum, start])) + Thread(target=work, args=[enum, start])) for t in worker_threads: t.start() for t in worker_threads: From 167d606b1c564c800f3f4db00e69a35584862758 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 4 Jan 2025 12:52:07 +0100 Subject: [PATCH 09/16] refactor to avoid returning from within a critical section --- Objects/enumobject.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/Objects/enumobject.c b/Objects/enumobject.c index 83cbcacbac36cf..15c8665346ca1a 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -168,32 +168,43 @@ enum_traverse(enumobject *en, visitproc visit, void *arg) return 0; } +// increment en_longindex with lock held, return the next index to be used +// or NULL on error +static inline PyObject * +increment_longindex_lock_held(enumobject *en) +{ + PyObject *next_index = en->en_longindex; + if (next_index == NULL) { + next_index = PyLong_FromSsize_t(PY_SSIZE_T_MAX); + if (next_index == NULL) { + return NULL; + } + } + assert(next_index != NULL); + PyObject *stepped_up = PyNumber_Add(next_index, en->one); + if (stepped_up == NULL) { + return NULL; + } + en->en_longindex = stepped_up; + return next_index; +} + static PyObject * enum_next_long(enumobject *en, PyObject* next_item) { PyObject *result = en->en_result; PyObject *next_index; - PyObject *stepped_up; PyObject *old_index; PyObject *old_item; + Py_BEGIN_CRITICAL_SECTION(en); - if (en->en_longindex == NULL) { - en->en_longindex = PyLong_FromSsize_t(PY_SSIZE_T_MAX); - if (en->en_longindex == NULL) { - Py_DECREF(next_item); - return NULL; - } - } - next_index = en->en_longindex; - assert(next_index != NULL); - stepped_up = PyNumber_Add(next_index, en->one); - if (stepped_up == NULL) { + next_index = increment_longindex_lock_held(en); + Py_END_CRITICAL_SECTION(); + if (next_index == NULL) { Py_DECREF(next_item); return NULL; } - en->en_longindex = stepped_up; - Py_END_CRITICAL_SECTION(); if (_PyObject_IsUniquelyReferenced(result)) { Py_INCREF(result); From 23c575d917b2f35d3d04df604b8985580b2d2c7f Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 4 Jan 2025 12:59:51 +0100 Subject: [PATCH 10/16] make sure all threads start iterating at the same moment in the test --- Lib/test/test_free_threading/test_enumerate.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index bcc6588d34fca5..7e5ba63e59684a 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -14,22 +14,25 @@ def test_threading(self): number_of_iterations = 8 n = 100 start = sys.maxsize - 40 - - def work(enum, start): + workers_enabled = False + def work(enum): while True: - try: - _ = next(enum) - except StopIteration: - break + if workers_enabled: + print('go!') + try: + _ = next(enum) + except StopIteration: + break for _ in range(number_of_iterations): enum = enumerate(range(start, start + n)) worker_threads = [] for ii in range(number_of_threads): worker_threads.append( - Thread(target=work, args=[enum, start])) + Thread(target=work, args=[enum])) for t in worker_threads: t.start() + workers_enabled = True # make sure to start all threads simultaneously for t in worker_threads: t.join() From 7450a34fc15de4f7ea9dd3435fc1e4cfef2c28c2 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 4 Jan 2025 13:03:19 +0100 Subject: [PATCH 11/16] remove debug statement --- Lib/test/test_free_threading/test_enumerate.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index 7e5ba63e59684a..861c86481a35a8 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -18,7 +18,6 @@ def test_threading(self): def work(enum): while True: if workers_enabled: - print('go!') try: _ = next(enum) except StopIteration: From 8dd07015b5cdf647576afb923d2f4d3d8827a41d Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Wed, 15 Jan 2025 20:55:57 +0100 Subject: [PATCH 12/16] Update Objects/enumobject.c Co-authored-by: Sam Gross --- Objects/enumobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/enumobject.c b/Objects/enumobject.c index ea51b33ececae8..88c3fd236b9d18 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -291,7 +291,7 @@ static PyObject * enum_reduce(PyObject *op, PyObject *Py_UNUSED(ignored)) { enumobject *en = _enumobject_CAST(op); - PyObject * result; + PyObject *result; Py_BEGIN_CRITICAL_SECTION(en); if (en->en_longindex != NULL) result = Py_BuildValue("O(OO)", Py_TYPE(en), en->en_sit, en->en_longindex); From a90d5e8ced6c98baed6154b4dd253aa99a9fc24e Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Wed, 15 Jan 2025 21:03:55 +0100 Subject: [PATCH 13/16] review suggestions --- Lib/test/test_free_threading/test_enumerate.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index 861c86481a35a8..487f3cc7bf72f3 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -1,6 +1,6 @@ import unittest import sys -from threading import Thread +from threading import Thread, Barrier from test.support import threading_helper @@ -14,14 +14,14 @@ def test_threading(self): number_of_iterations = 8 n = 100 start = sys.maxsize - 40 - workers_enabled = False + barrier = Barrier(number_of_threads) def work(enum): + barrier.wait() while True: - if workers_enabled: - try: - _ = next(enum) - except StopIteration: - break + try: + _ = next(enum) + except StopIteration: + break for _ in range(number_of_iterations): enum = enumerate(range(start, start + n)) @@ -31,10 +31,10 @@ def work(enum): Thread(target=work, args=[enum])) for t in worker_threads: t.start() - workers_enabled = True # make sure to start all threads simultaneously for t in worker_threads: t.join() + barrier.reset() if __name__ == "__main__": unittest.main() From 9b400390edceea470c3ad7a5899f846b62ef0a11 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Wed, 15 Jan 2025 21:53:42 +0100 Subject: [PATCH 14/16] fix test --- Lib/test/test_free_threading/test_enumerate.py | 6 +++--- Objects/object.c | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index 487f3cc7bf72f3..13b94ef9f7152d 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -10,7 +10,7 @@ class EnumerateThreading(unittest.TestCase): @threading_helper.reap_threads @threading_helper.requires_working_threading() def test_threading(self): - number_of_threads = 4 + number_of_threads = 10 number_of_iterations = 8 n = 100 start = sys.maxsize - 40 @@ -23,8 +23,8 @@ def work(enum): except StopIteration: break - for _ in range(number_of_iterations): - enum = enumerate(range(start, start + n)) + for it in range(number_of_iterations): + enum = enumerate(tuple(range(start, start + n))) worker_threads = [] for ii in range(number_of_threads): worker_threads.append( diff --git a/Objects/object.c b/Objects/object.c index 9befd92e3231c8..dfef1397ae3a27 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -264,6 +264,7 @@ _Py_AddToAllObjects(PyObject *op) void _Py_NegativeRefcount(const char *filename, int lineno, PyObject *op) { + printf("negative refcount for object of type %s\n", Py_TYPE(op)->tp_name); _PyObject_AssertFailed(op, NULL, "object has negative ref count", filename, lineno, __func__); } From 87cbd42ddb42d4554a313eb08b494ece38eae9e5 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Wed, 15 Jan 2025 22:21:18 +0100 Subject: [PATCH 15/16] cleanup test --- Objects/object.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Objects/object.c b/Objects/object.c index dfef1397ae3a27..9befd92e3231c8 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -264,7 +264,6 @@ _Py_AddToAllObjects(PyObject *op) void _Py_NegativeRefcount(const char *filename, int lineno, PyObject *op) { - printf("negative refcount for object of type %s\n", Py_TYPE(op)->tp_name); _PyObject_AssertFailed(op, NULL, "object has negative ref count", filename, lineno, __func__); } From 2af6ce93b5589554d30396ad26006a694df61d66 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Wed, 12 Mar 2025 21:25:03 +0100 Subject: [PATCH 16/16] refactor test --- Lib/test/test_free_threading/test_enumerate.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_free_threading/test_enumerate.py b/Lib/test/test_free_threading/test_enumerate.py index 13b94ef9f7152d..d23f5e9529f4c2 100644 --- a/Lib/test/test_free_threading/test_enumerate.py +++ b/Lib/test/test_free_threading/test_enumerate.py @@ -4,11 +4,11 @@ from test.support import threading_helper +threading_helper.requires_working_threading(module=True) class EnumerateThreading(unittest.TestCase): @threading_helper.reap_threads - @threading_helper.requires_working_threading() def test_threading(self): number_of_threads = 10 number_of_iterations = 8 @@ -29,10 +29,8 @@ def work(enum): for ii in range(number_of_threads): worker_threads.append( Thread(target=work, args=[enum])) - for t in worker_threads: - t.start() - for t in worker_threads: - t.join() + with threading_helper.start_threads(worker_threads): + pass barrier.reset()