Add _PyType_Fetch and use incref before setting attribute on type

Makes setting an attribute on a class and signaling type modified atomic
Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated

Include/cpython/object.h

@@ -275,6 +275,7 @@ typedef struct _heaptypeobject {
 
 PyAPI_FUNC(const char *) _PyType_Name(PyTypeObject *);
 PyAPI_FUNC(PyObject *) _PyType_Lookup(PyTypeObject *, PyObject *);
+PyAPI_FUNC(PyObject *) _PyType_Fetch(PyTypeObject *, PyObject *);
 PyAPI_FUNC(PyObject *) PyType_GetDict(PyTypeObject *);
 
 PyAPI_FUNC(int) PyObject_Print(PyObject *, FILE *, int);

Include/cpython/pyatomic.h

@@ -484,6 +484,9 @@ _Py_atomic_store_int_release(int *obj, int value);
 static inline int
 _Py_atomic_load_int_acquire(const int *obj);
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value);
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value);
 

Include/cpython/pyatomic_gcc.h

@@ -516,6 +516,10 @@ static inline int
 _Py_atomic_load_int_acquire(const int *obj)
 { return __atomic_load_n(obj, __ATOMIC_ACQUIRE); }
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
+{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
 { __atomic_store_n(obj, value, __ATOMIC_RELEASE); }

Include/cpython/pyatomic_msc.h

@@ -989,6 +989,19 @@ _Py_atomic_load_int_acquire(const int *obj)
 #endif
 }
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
+{
+#if defined(_M_X64) || defined(_M_IX86)
+    *(uint32_t volatile *)obj = value;
+#elif defined(_M_ARM64)
+    _Py_atomic_ASSERT_ARG_TYPE(unsigned __int32);
+    __stlr32((unsigned __int32 volatile *)obj, (unsigned __int32)value);
+#else
+#  error "no implementation of _Py_atomic_store_uint32_release"
+#endif
+}
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
 {

Include/cpython/pyatomic_std.h

@@ -911,6 +911,14 @@ _Py_atomic_load_int_acquire(const int *obj)
                                 memory_order_acquire);
 }
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
+{
+    _Py_USING_STD;
+    atomic_store_explicit((_Atomic(uint32_t)*)obj, value,
+                          memory_order_release);
+}
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
 {

Include/internal/pycore_dict.h

@@ -106,6 +106,9 @@ PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObjec
 /* Consumes references to key and value */
 PyAPI_FUNC(int) _PyDict_SetItem_Take2(PyDictObject *op, PyObject *key, PyObject *value);
 extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr, PyObject *name, PyObject *value);
+extern int _PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value);
+extern int _PyDict_GetItemRef_LockHeld(PyObject *op, PyObject *key, PyObject **result);
+extern int _PyDict_GetItemRef_KnownHash(PyObject *op, PyObject *key, Py_hash_t hash, PyObject **result);
 
 extern int _PyDict_Pop_KnownHash(
     PyDictObject *dict,

Lib/test/test_class.py

@@ -882,6 +882,25 @@ class Foo:
         f.a = 3
         self.assertEqual(f.a, 3)
 
+    def test_store_attr_type_cache(self):
+        """Verifies that the type cache doesn't provide a value which  is
+        inconsistent from the dict."""
+        class X:
+            def __del__(inner_self):
+                v = C.a
+                self.assertEqual(v, C.__dict__['a'])
+
+        class C:
+            a = X()
+
+        # prime the cache
+        C.a
+        C.a
+
+        # destructor shouldn't be able to see inconsisent state
+        C.a = X()
+        C.a = X()
+
 
 if __name__ == '__main__':
     unittest.main()

Lib/test/test_free_threading/test_type.py

@@ -0,0 +1,112 @@
+import unittest
+
+from threading import Thread
+from multiprocessing.dummy import Pool
+from unittest import TestCase
+
+from test.support import is_wasi
+
+
+NTHREADS = 6
+BOTTOM = 0
+TOP = 1000
+ITERS = 100
+
+class A:
+    attr = 1
+
+@unittest.skipIf(is_wasi, "WASI has no threads.")
+class TestType(TestCase):
+    def test_attr_cache(self):
+        def read(id0):
+            for _ in range(ITERS):
+                for _ in range(BOTTOM, TOP):
+                    A.attr
+
+        def write(id0):
+            for _ in range(ITERS):
+                for _ in range(BOTTOM, TOP):
+                    # Make _PyType_Lookup cache hot first
+                    A.attr
+                    A.attr
+                    x = A.attr
+                    x += 1
+                    A.attr = x
+
+
+        with Pool(NTHREADS) as pool:
+            pool.apply_async(read, (1,))
+            pool.apply_async(write, (1,))
+            pool.close()
+            pool.join()
+
+    def test_attr_cache_consistency(self):
+        class C:
+            x = 0
+
+        DONE = False
+        def writer_func():
+            for i in range(10000):
+                C.x
+                C.x
+                C.x += 1
+            nonlocal DONE
+            DONE = True
+
+        def reader_func():
+            while True:
+                # We should always see a greater value read from the type than the
+                # dictionary
+                a = C.__dict__['x']
+                b = C.x
+                self.assertGreaterEqual(b, a)
+
+                if DONE:
+                    break
+
+        self.run_one(writer_func, reader_func)
+
+    def test_attr_cache_consistency_subclass(self):
+        class C:
+            x = 0
+
+        class D(C):
+            pass
+
+        DONE = False
+        def writer_func():
+            for i in range(10000):
+                D.x
+                D.x
+                C.x += 1
+            nonlocal DONE
+            DONE = True
+
+        def reader_func():
+            while True:
+                # We should always see a greater value read from the type than the
+                # dictionary
+                a = C.__dict__['x']
+                b = D.x
+                self.assertGreaterEqual(b, a)
+
+                if DONE:
+                    break
+
+        self.run_one(writer_func, reader_func)
+
+    def run_one(self, writer_func, reader_func):
+        writer = Thread(target=writer_func)
+        readers = []
+        for x in range(30):
+            reader = Thread(target=reader_func)
+            readers.append(reader)
+            reader.start()
+
+        writer.start()
+        writer.join()
+        for reader in readers:
+            reader.join()
+
+if __name__ == "__main__":
+    unittest.main()

Modules/_collectionsmodule.c

@@ -2511,9 +2511,9 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
     /* Only take the fast path when get() and __setitem__()
      * have not been overridden.
      */
-    mapping_get = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(get));
+    mapping_get = _PyType_Fetch(Py_TYPE(mapping), &_Py_ID(get));
     dict_get = _PyType_Lookup(&PyDict_Type, &_Py_ID(get));
-    mapping_setitem = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(__setitem__));
+    mapping_setitem = _PyType_Fetch(Py_TYPE(mapping), &_Py_ID(__setitem__));
     dict_setitem = _PyType_Lookup(&PyDict_Type, &_Py_ID(__setitem__));
 
     if (mapping_get != NULL && mapping_get == dict_get &&
@@ -2587,6 +2587,8 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
     }
 
 done:
+    Py_XDECREF(mapping_get);
+    Py_XDECREF(mapping_setitem);
     Py_DECREF(it);
     Py_XDECREF(key);
     Py_XDECREF(newval);

Modules/_lsprof.c

@@ -175,8 +175,7 @@ normalizeUserObj(PyObject *obj)
         PyObject *modname = fn->m_module;
 
         if (name != NULL) {
-            PyObject *mo = _PyType_Lookup(Py_TYPE(self), name);
-            Py_XINCREF(mo);
+            PyObject *mo = _PyType_Fetch(Py_TYPE(self), name);
             Py_DECREF(name);
             if (mo != NULL) {
                 PyObject *res = PyObject_Repr(mo);

Modules/_testcapi/gc.c

@@ -99,10 +99,11 @@ slot_tp_del(PyObject *self)
         return;
     }
     /* Execute __del__ method, if any. */
-    del = _PyType_Lookup(Py_TYPE(self), tp_del);
+    del = _PyType_Fetch(Py_TYPE(self), tp_del);
     Py_DECREF(tp_del);
     if (del != NULL) {
         res = PyObject_CallOneArg(del, self);
+        Py_DECREF(del);
         if (res == NULL)
             PyErr_WriteUnraisable(del);
         else

Objects/classobject.c

@@ -188,15 +188,18 @@ method_getattro(PyObject *obj, PyObject *name)
             if (PyType_Ready(tp) < 0)
                 return NULL;
         }
-        descr = _PyType_Lookup(tp, name);
+        descr = _PyType_Fetch(tp, name);
     }
 
     if (descr != NULL) {
         descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
-        if (f != NULL)
-            return f(descr, obj, (PyObject *)Py_TYPE(obj));
+        if (f != NULL) {
+            PyObject *res = f(descr, obj, (PyObject *)Py_TYPE(obj));
+            Py_DECREF(descr);
+            return res;
+        }
         else {
-            return Py_NewRef(descr);
+            return descr;
         }
     }
 
@@ -410,14 +413,17 @@ instancemethod_getattro(PyObject *self, PyObject *name)
         if (PyType_Ready(tp) < 0)
             return NULL;
     }
-    descr = _PyType_Lookup(tp, name);
+    descr = _PyType_Fetch(tp, name);
 
     if (descr != NULL) {
         descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
-        if (f != NULL)
-            return f(descr, self, (PyObject *)Py_TYPE(self));
+        if (f != NULL) {
+            PyObject *res = f(descr, self, (PyObject *)Py_TYPE(self));
+            Py_DECREF(descr);
+            return res;
+        }
         else {
-            return Py_NewRef(descr);
+            return descr;
         }
     }
 

Objects/dictobject.c

@@ -2317,6 +2317,37 @@ PyDict_GetItemRef(PyObject *op, PyObject *key, PyObject **result)
     return _PyDict_GetItemRef_KnownHash(op, key, hash, result);
 }
 
+int
+_PyDict_GetItemRef_LockHeld(PyObject *op, PyObject *key, PyObject **result)
+{
+    ASSERT_DICT_LOCKED(op);
+    assert(PyUnicode_CheckExact(key));
+
+    Py_hash_t hash;
+    if ((hash = unicode_get_hash(key)) == -1) {
+        hash = PyObject_Hash(key);
+        if (hash == -1) {
+            *result = NULL;
+            return -1;
+        }
+    }
+
+    PyDictObject*mp = (PyDictObject *)op;
+
+    PyObject *value;
+    Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, &value);
+    assert(ix >= 0 || value == NULL);
+    if (ix == DKIX_ERROR) {
+        *result = NULL;
+        return -1;
+    }
+    if (value == NULL) {
+        *result = NULL;
+        return 0;  // missing key
+    }
+    *result = Py_NewRef(value);
+    return 1;  // key is present
+}
 
 /* Variant of PyDict_GetItem() that doesn't suppress exceptions.
    This returns NULL *with* an exception set if an exception occurred.
@@ -6704,8 +6735,8 @@ _PyObject_MaterializeManagedDict(PyObject *obj)
     return dict;
 }
 
-static int
-set_or_del_lock_held(PyDictObject *dict, PyObject *name, PyObject *value)
+int
+_PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value)
 {
     if (value == NULL) {
         Py_hash_t hash;
@@ -6767,7 +6798,7 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values,
             // so that no one else will see it.
             dict = make_dict_from_instance_attributes(PyInterpreterState_Get(), keys, values);
             if (dict == NULL ||
-                set_or_del_lock_held(dict, name, value) < 0) {
+                _PyDict_SetItem_LockHeld(dict, name, value) < 0) {
                 Py_XDECREF(dict);
                 return -1;
             }
@@ -6779,7 +6810,7 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values,
 
         _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(dict);
 
-        res = set_or_del_lock_held (dict, name, value);
+        res = _PyDict_SetItem_LockHeld(dict, name, value);
         return res;
     }
 
@@ -6822,7 +6853,7 @@ store_instance_attr_dict(PyObject *obj, PyDictObject *dict, PyObject *name, PyOb
         res = store_instance_attr_lock_held(obj, values, name, value);
     }
     else {
-        res = set_or_del_lock_held(dict, name, value);
+        res = _PyDict_SetItem_LockHeld(dict, name, value);
     }
     Py_END_CRITICAL_SECTION();
     return res;
@@ -7253,6 +7284,7 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr,
             res = PyDict_SetItem(dict, key, value);
         }
     }
+
     ASSERT_CONSISTENT(dict);
     return res;
 }