From df767e58c97f074c6391a228ff3098cabbbdde29 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Thu, 6 Jun 2024 23:49:38 +0800 Subject: [PATCH 01/11] Make Py_TYPE and Py_SET_TYPE thread safe --- Include/object.h | 8 ++++++++ Tools/tsan/suppressions_free_threading.txt | 2 -- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Include/object.h b/Include/object.h index c8c63b9b2b1450..bbc17f574b341b 100644 --- a/Include/object.h +++ b/Include/object.h @@ -246,7 +246,11 @@ _Py_IsOwnedByCurrentThread(PyObject *ob) // bpo-39573: The Py_SET_TYPE() function must be used to set an object type. static inline PyTypeObject* Py_TYPE(PyObject *ob) { +#ifdef Py_GIL_DISABLED + return _Py_atomic_load_ptr_relaxed(&ob->ob_type); +#else return ob->ob_type; +#endif } #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000 # define Py_TYPE(ob) Py_TYPE(_PyObject_CAST(ob)) @@ -274,7 +278,11 @@ static inline int Py_IS_TYPE(PyObject *ob, PyTypeObject *type) { static inline void Py_SET_TYPE(PyObject *ob, PyTypeObject *type) { +#ifdef Py_GIL_DISABLED + _Py_atomic_store_ptr(&ob->ob_type, type); +#else ob->ob_type = type; +#endif } #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000 # define Py_SET_TYPE(ob, type) Py_SET_TYPE(_PyObject_CAST(ob), type) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 8b64d1ff321858..4324f9db8922dc 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -39,7 +39,6 @@ race_top:set_contains_key # https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35 race_top:set_discard_entry race_top:set_inheritable -race_top:Py_SET_TYPE race_top:_PyDict_CheckConsistency race_top:_PyImport_AcquireLock race_top:_Py_dict_lookup_threadsafe @@ -62,7 +61,6 @@ race_top:_PyFrame_Initialize race_top:PyInterpreterState_ThreadHead race_top:_PyObject_TryGetInstanceAttribute race_top:PyThreadState_Next -race_top:Py_TYPE race_top:PyUnstable_InterpreterFrame_GetLine race_top:sock_close race_top:tstate_delete_common From 2e9fda6d71bd90a16bdee8ed57b109d2eb3cfc6d Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 00:05:33 +0800 Subject: [PATCH 02/11] fix Windows --- Include/object.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/object.h b/Include/object.h index bbc17f574b341b..4a39ada8c7daa4 100644 --- a/Include/object.h +++ b/Include/object.h @@ -247,7 +247,7 @@ _Py_IsOwnedByCurrentThread(PyObject *ob) // bpo-39573: The Py_SET_TYPE() function must be used to set an object type. static inline PyTypeObject* Py_TYPE(PyObject *ob) { #ifdef Py_GIL_DISABLED - return _Py_atomic_load_ptr_relaxed(&ob->ob_type); + return (PyTypeObject *)_Py_atomic_load_ptr_relaxed(&ob->ob_type); #else return ob->ob_type; #endif From bda9545ac413922c49b91fad870e12990258ffac Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 12:28:11 +0800 Subject: [PATCH 03/11] add test --- Include/internal/pycore_interp.h | 7 ++++-- Lib/test/test_free_threading/test_type.py | 27 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 86dada5061e7b5..2fecc08c2facee 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -400,8 +400,11 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New( #define RARE_EVENT_INTERP_INC(interp, name) \ do { \ - /* saturating add */ \ - if (interp->rare_events.name < UINT8_MAX) interp->rare_events.name++; \ + /* saturating add */ \ + int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \ + if (val < UINT8_MAX) { \ + FT_ATOMIC_STORE_UINT8_RELAXED(interp->rare_events.name, val+1); \ + } \ RARE_EVENT_STAT_INC(name); \ } while (0); \ diff --git a/Lib/test/test_free_threading/test_type.py b/Lib/test/test_free_threading/test_type.py index 6eead198deed46..1e84b2db2d4882 100644 --- a/Lib/test/test_free_threading/test_type.py +++ b/Lib/test/test_free_threading/test_type.py @@ -1,3 +1,4 @@ +import threading import unittest from concurrent.futures import ThreadPoolExecutor @@ -95,6 +96,32 @@ def reader_func(): self.run_one(writer_func, reader_func) + def test___class___modification(self): + class Foo: + pass + + class Bar: + pass + + thing = Foo() + def work(): + foo = thing + for _ in range(10000): + foo.__class__ = Bar + type(foo) + foo.__class__ = Foo + type(foo) + + + threads = [] + for i in range(NTHREADS): + thread = threading.Thread(target=work) + thread.start() + threads.append(thread) + + for thread in threads: + thread.join() + def run_one(self, writer_func, reader_func): writer = Thread(target=writer_func) readers = [] From b49dc1a2ce41925a6b869e30453e36a868fd733d Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 12:34:14 +0800 Subject: [PATCH 04/11] style nits --- Include/internal/pycore_interp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 2fecc08c2facee..2a3ae6be914a45 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -400,7 +400,7 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New( #define RARE_EVENT_INTERP_INC(interp, name) \ do { \ - /* saturating add */ \ + /* saturating add */ \ int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \ if (val < UINT8_MAX) { \ FT_ATOMIC_STORE_UINT8_RELAXED(interp->rare_events.name, val+1); \ From 54553a9ca63501d26cbcb2ce51106f254e5aa9b9 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 14:42:43 +0800 Subject: [PATCH 05/11] remove free-threaded stuff temporarily --- Include/internal/pycore_interp.h | 5 +---- Include/object.h | 8 -------- Tools/tsan/suppressions_free_threading.txt | 2 ++ 3 files changed, 3 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 2a3ae6be914a45..86dada5061e7b5 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -401,10 +401,7 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New( #define RARE_EVENT_INTERP_INC(interp, name) \ do { \ /* saturating add */ \ - int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \ - if (val < UINT8_MAX) { \ - FT_ATOMIC_STORE_UINT8_RELAXED(interp->rare_events.name, val+1); \ - } \ + if (interp->rare_events.name < UINT8_MAX) interp->rare_events.name++; \ RARE_EVENT_STAT_INC(name); \ } while (0); \ diff --git a/Include/object.h b/Include/object.h index 4a39ada8c7daa4..c8c63b9b2b1450 100644 --- a/Include/object.h +++ b/Include/object.h @@ -246,11 +246,7 @@ _Py_IsOwnedByCurrentThread(PyObject *ob) // bpo-39573: The Py_SET_TYPE() function must be used to set an object type. static inline PyTypeObject* Py_TYPE(PyObject *ob) { -#ifdef Py_GIL_DISABLED - return (PyTypeObject *)_Py_atomic_load_ptr_relaxed(&ob->ob_type); -#else return ob->ob_type; -#endif } #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000 # define Py_TYPE(ob) Py_TYPE(_PyObject_CAST(ob)) @@ -278,11 +274,7 @@ static inline int Py_IS_TYPE(PyObject *ob, PyTypeObject *type) { static inline void Py_SET_TYPE(PyObject *ob, PyTypeObject *type) { -#ifdef Py_GIL_DISABLED - _Py_atomic_store_ptr(&ob->ob_type, type); -#else ob->ob_type = type; -#endif } #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000 # define Py_SET_TYPE(ob, type) Py_SET_TYPE(_PyObject_CAST(ob), type) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 4324f9db8922dc..8b64d1ff321858 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -39,6 +39,7 @@ race_top:set_contains_key # https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35 race_top:set_discard_entry race_top:set_inheritable +race_top:Py_SET_TYPE race_top:_PyDict_CheckConsistency race_top:_PyImport_AcquireLock race_top:_Py_dict_lookup_threadsafe @@ -61,6 +62,7 @@ race_top:_PyFrame_Initialize race_top:PyInterpreterState_ThreadHead race_top:_PyObject_TryGetInstanceAttribute race_top:PyThreadState_Next +race_top:Py_TYPE race_top:PyUnstable_InterpreterFrame_GetLine race_top:sock_close race_top:tstate_delete_common From b1dc75bb2220c72f3ad0a5fad4eb87cfe2e74ad7 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 16:51:12 +0800 Subject: [PATCH 06/11] Remove heap type flags --- Objects/typeobject.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 880ac6b9c009fe..8aa04b1cadc381 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6573,13 +6573,7 @@ object_set_class(PyObject *self, PyObject *value, void *closure) return -1; } } - if (newto->tp_flags & Py_TPFLAGS_HEAPTYPE) { - Py_INCREF(newto); - } Py_SET_TYPE(self, newto); - if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) - Py_DECREF(oldto); - RARE_EVENT_INC(set_class); return 0; } From 5523a9e166d9569a4ce21ace10e2fc86fe95e77c Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 16:57:48 +0800 Subject: [PATCH 07/11] Revert "Remove heap type flags" This reverts commit b1dc75bb2220c72f3ad0a5fad4eb87cfe2e74ad7. --- Objects/typeobject.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 8aa04b1cadc381..880ac6b9c009fe 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6573,7 +6573,13 @@ object_set_class(PyObject *self, PyObject *value, void *closure) return -1; } } + if (newto->tp_flags & Py_TPFLAGS_HEAPTYPE) { + Py_INCREF(newto); + } Py_SET_TYPE(self, newto); + if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) + Py_DECREF(oldto); + RARE_EVENT_INC(set_class); return 0; } From 02a558783e619109f837f61b6ae050dac09831b6 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Mon, 10 Jun 2024 23:05:23 +0800 Subject: [PATCH 08/11] fix bug Co-Authored-By: Nadeshiko Manju --- Objects/typeobject.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 880ac6b9c009fe..0383918740d1e3 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6466,7 +6466,6 @@ compatible_for_assignment(PyTypeObject* oldto, PyTypeObject* newto, const char* static int object_set_class(PyObject *self, PyObject *value, void *closure) { - PyTypeObject *oldto = Py_TYPE(self); if (value == NULL) { PyErr_SetString(PyExc_TypeError, @@ -6486,6 +6485,8 @@ object_set_class(PyObject *self, PyObject *value, void *closure) return -1; } + PyTypeObject *oldto = Py_TYPE(self); + /* In versions of CPython prior to 3.5, the code in compatible_for_assignment was not set up to correctly check for memory layout / slot / etc. compatibility for non-HEAPTYPE classes, so we just From 70523eb60995da91431b622f81cdbaf1e5df07f6 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 10 Jun 2024 15:07:20 +0000 Subject: [PATCH 09/11] =?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-06-10-15-07-16.gh-issue-120198.WW_pjO.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-06-10-15-07-16.gh-issue-120198.WW_pjO.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-10-15-07-16.gh-issue-120198.WW_pjO.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-10-15-07-16.gh-issue-120198.WW_pjO.rst new file mode 100644 index 00000000000000..8dc8aec44d80c4 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-10-15-07-16.gh-issue-120198.WW_pjO.rst @@ -0,0 +1 @@ +Fix a crash when multiple threads read and write to the same ``__class__`` of an object concurrently. From 1fc5c01e40af797d786a8e49f4cf7d2998edabb4 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Mon, 10 Jun 2024 23:29:19 +0800 Subject: [PATCH 10/11] Move test like Steve suggested Co-Authored-By: Steve Dower --- Lib/test/test_free_threading/test_type.py | 26 ------------------ Lib/test/test_super.py | 32 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/Lib/test/test_free_threading/test_type.py b/Lib/test/test_free_threading/test_type.py index 1e84b2db2d4882..786336fa0cddce 100644 --- a/Lib/test/test_free_threading/test_type.py +++ b/Lib/test/test_free_threading/test_type.py @@ -96,32 +96,6 @@ def reader_func(): self.run_one(writer_func, reader_func) - def test___class___modification(self): - class Foo: - pass - - class Bar: - pass - - thing = Foo() - def work(): - foo = thing - for _ in range(10000): - foo.__class__ = Bar - type(foo) - foo.__class__ = Foo - type(foo) - - - threads = [] - for i in range(NTHREADS): - thread = threading.Thread(target=work) - thread.start() - threads.append(thread) - - for thread in threads: - thread.join() - def run_one(self, writer_func, reader_func): writer = Thread(target=writer_func) readers = [] diff --git a/Lib/test/test_super.py b/Lib/test/test_super.py index 256b416caaa584..0b7289cf2bee67 100644 --- a/Lib/test/test_super.py +++ b/Lib/test/test_super.py @@ -1,6 +1,7 @@ """Unit tests for zero-argument super() & related machinery.""" import textwrap +import threading import unittest from unittest.mock import patch from test.support import import_helper @@ -505,6 +506,37 @@ def some(cls): for _ in range(ADAPTIVE_WARMUP_DELAY): C.some(C) + def test___class___modification_multithreaded(self): + """ Note: this test isn't actually testing anything on its own. + It requires a sys audithook to be set to crash on older Python. + This should be the case anyways as our test suite sets + an audit hook. + """ + class Foo: + pass + + class Bar: + pass + + thing = Foo() + def work(): + foo = thing + for _ in range(5000): + foo.__class__ = Bar + type(foo) + foo.__class__ = Foo + type(foo) + + + threads = [] + for _ in range(6): + thread = threading.Thread(target=work) + thread.start() + threads.append(thread) + + for thread in threads: + thread.join() + if __name__ == "__main__": unittest.main() From b70dc0720cb2447e94a58061b485ecfee3a7971c Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Tue, 11 Jun 2024 02:36:59 +0800 Subject: [PATCH 11/11] skip test if no threads --- Lib/test/test_super.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_super.py b/Lib/test/test_super.py index 0b7289cf2bee67..3ffbe03f0c2f11 100644 --- a/Lib/test/test_super.py +++ b/Lib/test/test_super.py @@ -4,7 +4,7 @@ import threading import unittest from unittest.mock import patch -from test.support import import_helper +from test.support import import_helper, threading_helper ADAPTIVE_WARMUP_DELAY = 2 @@ -506,6 +506,7 @@ def some(cls): for _ in range(ADAPTIVE_WARMUP_DELAY): C.some(C) + @threading_helper.requires_working_threading() def test___class___modification_multithreaded(self): """ Note: this test isn't actually testing anything on its own. It requires a sys audithook to be set to crash on older Python.