Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-117657: Make Py_TYPE and Py_SET_TYPE thread safe #120165

Merged
merged 9 commits into from
Jun 12, 2024
Merged

gh-117657: Make Py_TYPE and Py_SET_TYPE thread safe #120165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
df767e5
Make Py_TYPE and Py_SET_TYPE thread safe
Fidget-Spinner Jun 6, 2024
2e9fda6
fix Windows
Fidget-Spinner Jun 6, 2024
bda9545
add test
Fidget-Spinner Jun 7, 2024
b49dc1a
style nits
Fidget-Spinner Jun 7, 2024
07459a1
use stronger guarantees
Fidget-Spinner Jun 9, 2024
156908d
Update Include/internal/pycore_interp.h
Fidget-Spinner Jun 9, 2024
af62c36
Merge remote-tracking branch 'upstream/main' into pytype_threadsafe
Fidget-Spinner Jun 12, 2024
9a33a78
Lock critical section
Fidget-Spinner Jun 12, 2024
e43acf0
shrink critical section
Fidget-Spinner Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Include/internal/pycore_interp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,10 @@ 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++; \
int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \
if (val < UINT8_MAX) { \
FT_ATOMIC_STORE_UINT8(interp->rare_events.name, val + 1); \
} \
RARE_EVENT_STAT_INC(name); \
} while (0); \

Expand Down
8 changes: 8 additions & 0 deletions Include/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (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))
Expand Down Expand Up @@ -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)
Expand Down
26 changes: 26 additions & 0 deletions Lib/test/test_free_threading/test_type.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,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 = []
Expand Down
8 changes: 7 additions & 1 deletion Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6633,9 +6633,15 @@ object_set_class(PyObject *self, PyObject *value, void *closure)
if (newto->tp_flags & Py_TPFLAGS_HEAPTYPE) {
Py_INCREF(newto);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it important to do the INCREF(newto) and DECREF(oldto) inside the critical section?

Or it it possible to restrict the critical section to the following code?

oldto = Py_TYPE(self);
Py_SET_TYPE(self, newto);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I can shrink it!

}
Py_BEGIN_CRITICAL_SECTION(self);
// The real Py_TYPE(self) (`oldto`) may have changed from
// underneath us in another thread, so we re-fetch it here.
oldto = Py_TYPE(self);
Py_SET_TYPE(self, newto);
if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE)
Py_END_CRITICAL_SECTION();
if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) {
Py_DECREF(oldto);
}

RARE_EVENT_INC(set_class);
return 0;
Expand Down
2 changes: 0 additions & 2 deletions Tools/tsan/suppressions_free_threading.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,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:_Py_dict_lookup_threadsafe
race_top:_multiprocessing_SemLock_acquire_impl
Expand All @@ -58,7 +57,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
Expand Down
Loading