Skip to content

Commit

Permalink
Incref dict when it's usage is borrowed
Browse files Browse the repository at this point in the history
  • Loading branch information
DinoV committed Jul 31, 2024
1 parent a64b6e0 commit fba0d7c
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 2 deletions.
43 changes: 43 additions & 0 deletions Lib/test/test_free_threading/test_dict.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,49 @@ def writer_func(l):
for ref in thread_list:
self.assertIsNone(ref())

def test_racing_set_object_dict(self):
"""Races assigning to __dict__ should be thread safe"""

class C: pass
f = C()
f.__dict__ = {"foo": 42}
THREAD_COUNT = 10
class MyDict(dict): pass

def writer_func():
for i in range(100000):
f.__dict__ = {"foo": 100}

def reader_func():
for i in range(100000):
f.foo

lists = []
readers = []
writers = []
for x in range(THREAD_COUNT):
thread_list = []
lists.append(thread_list)
writer = Thread(target=partial(writer_func))
writers.append(writer)

for x in range(THREAD_COUNT):
thread_list = []
lists.append(thread_list)
reader = Thread(target=partial(reader_func))
readers.append(reader)

for writer in writers:
writer.start()
for reader in readers:
reader.start()

for writer in writers:
writer.join()

for reader in readers:
reader.join()

@unittest.skipIf(_testcapi is None, 'need _testcapi module')
def test_dict_version(self):
dict_version = _testcapi.dict_version
Expand Down
24 changes: 23 additions & 1 deletion Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -7082,6 +7082,7 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
PyTypeObject *tp = Py_TYPE(obj);
if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) {
PyDictObject *dict = _PyObject_GetManagedDict(obj);
PyDictObject *locked_dict;

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Address sanitizer

unused variable ‘locked_dict’ [-Wunused-variable]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (1.1.1w)

unused variable ‘locked_dict’ [-Wunused-variable]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (3.0.13)

unused variable ‘locked_dict’ [-Wunused-variable]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (3.1.5)

unused variable ‘locked_dict’ [-Wunused-variable]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (3.2.1)

unused variable ‘locked_dict’ [-Wunused-variable]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Hypothesis tests on Ubuntu

unused variable ‘locked_dict’ [-Wunused-variable]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Ubuntu / build and test

unused variable ‘locked_dict’ [-Wunused-variable]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Windows / build (arm64)

'locked_dict': unreferenced local variable [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Windows / build (arm64)

'locked_dict': unreferenced local variable [D:\a\cpython\cpython\PCbuild\pythoncore.vcxproj]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Windows (free-threading) / build (arm64)

'locked_dict': unreferenced local variable [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Windows / build and test (x64)

'locked_dict': unreferenced local variable [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Windows / build and test (x64)

'locked_dict': unreferenced local variable [D:\a\cpython\cpython\PCbuild\pythoncore.vcxproj]

Check warning on line 7085 in Objects/dictobject.c

View workflow job for this annotation

GitHub Actions / Windows (free-threading) / build and test (x64)

'locked_dict': unreferenced local variable [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]
if (dict == NULL) {
#ifdef Py_GIL_DISABLED
Py_BEGIN_CRITICAL_SECTION(obj);
Expand All @@ -7091,6 +7092,9 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
set_dict_inline_values(obj, (PyDictObject *)new_dict);
}

Py_XINCREF(dict);
locked_dict = dict;

Py_END_CRITICAL_SECTION();

if (dict == NULL) {
Expand All @@ -7102,6 +7106,21 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
#endif
}

#ifdef Py_GIL_DISABLED
else {
// We only have a borrowed reference to the dict, so we
// need to incref it.
locked_dict = dict;
if (!_Py_TryIncref((PyObject *)locked_dict)) {
Py_BEGIN_CRITICAL_SECTION(obj);
locked_dict = _PyObject_GetManagedDict(obj);
Py_INCREF(locked_dict);
dict = locked_dict;
Py_END_CRITICAL_SECTION();
}
}
#endif

Py_BEGIN_CRITICAL_SECTION2(dict, obj);

// We've locked dict, but the actual dict could have changed
Expand All @@ -7115,7 +7134,10 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
Py_END_CRITICAL_SECTION2();

if (err == 0) {
Py_XDECREF(dict);
#ifdef Py_GIL_DISABLED
Py_DECREF(locked_dict); // Reference for keeping dict alive
#endif
Py_XDECREF(dict); // Reference we replaced in object
}
}
else {
Expand Down
15 changes: 14 additions & 1 deletion Objects/object.c
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

/* Generic object operations; and implementation of None */

#include "Python.h"
Expand Down Expand Up @@ -1618,7 +1617,21 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name,
}
}
if (dict != NULL) {
#ifdef Py_GIL_DISABLED
if (!_Py_TryIncref(dict)) {
Py_BEGIN_CRITICAL_SECTION(obj);
if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) {
dict = (PyObject *)_PyObject_GetManagedDict(obj);
}
else {
dict = *_PyObject_ComputedDictPointer(obj);
}
Py_INCREF(dict);
Py_END_CRITICAL_SECTION();
}
#else
Py_INCREF(dict);
#endif
int rc = PyDict_GetItemRef(dict, name, &res);
Py_DECREF(dict);
if (res != NULL) {
Expand Down

0 comments on commit fba0d7c

Please sign in to comment.