code review

kumaraditya303 · Mar 30, 2025 · Mar 25, 2025 · Mar 25, 2025 · Mar 25, 2025 · Mar 25, 2025
commit ef9ba7a6f940644ef1dcff124df5c8f73a34bb5b
diff --git a/Lib/test/test_ctypes/test_free_threading.py b/Lib/test/test_ctypes/test_free_threading.py
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
@@ -716,9 +716,6 @@ StructUnionType_init(PyObject *self, PyObject *args, PyObject *kwds, int isStruc
         /* copy base info */
         ret = PyCStgInfo_clone(info, baseinfo);
         if (ret >= 0) {
-            // clear the 'final' bit in the subclass info
-            // safe to modify without atomics as it is not exposed to other threads
-            info->dict_final = 0;
             stginfo_set_dict_final_lock_held(baseinfo); /* set the 'final' flag in the baseclass info */
         }
         STGINFO_UNLOCK();

diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h
@@ -406,21 +406,17 @@ typedef struct {
 } StgInfo;
 
 /*
-    In free-threading, concurrent mutations to StgInfo is not thread safe.
-    Therefore to make it thread safe, when modifying StgInfo, `STGINFO_LOCK` and
-    `STGINFO_UNLOCK` macros are used to acquire critical section of the StgInfo.
-    The critical section is write only and is acquired when modifying the
-    StgInfo fields and while setting the `dict_final` bit. Once the `dict_final`
-    is set, StgInfo is treated as read only and no further modifications are
-    allowed. This allows to avoid acquiring the critical section for most
-    read operations when `dict_final` is set (general case).
-
-    It is important to set all the fields before setting the `dict_final` bit
-    in functions like `PyCStructUnionType_update_stginfo` because the store of
-    `dict_final` uses sequential consistency memory ordering. This ensures that
-    all the other fields are visible to other threads before the `dict_final` bit
-    is set thus allowing for lock free reads when `dict_final` is set.
-
+    To ensure thread safety in the free threading build, the `STGINFO_LOCK` and
+    `STGINFO_UNLOCK` macros use critical sections to protect against concurrent
+    modifications to `StgInfo` and assignment of the `dict_final` field. Once
+    `dict_final` is set, `StgInfo` is treated as read-only, and no further
+    modifications are allowed. This approach allows most read operations to
+    proceed without acquiring the critical section lock.
+
+    The `dict_final` field is written only after all other modifications to
+    `StgInfo` are complete. The reads and writes of `dict_final` use the
+    sequentially consistent memory ordering to ensure that all other fields are
+    visible to other threads before the `dict_final` bit is set.
 */
 
 #define STGINFO_LOCK(stginfo)   Py_BEGIN_CRITICAL_SECTION_MUT(&(stginfo)->mutex)

diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c
@@ -37,6 +37,7 @@ PyCStgInfo_clone(StgInfo *dst_info, StgInfo *src_info)
 #ifdef Py_GIL_DISABLED
     dst_info->mutex = (PyMutex){0};
 #endif
+    dst_info->dict_final = 0;
 
     Py_XINCREF(dst_info->proto);
     Py_XINCREF(dst_info->argtypes);
@@ -264,16 +265,9 @@ PyCStructUnionType_update_stginfo(PyObject *type, PyObject *fields, int isStruct
         return -1;
     }
 
+    STGINFO_LOCK(stginfo);
     /* If this structure/union is already marked final we cannot assign
        _fields_ anymore. */
-
-    if (stginfo_get_dict_final(stginfo) == 1) {/* is final ? */
-        PyErr_SetString(PyExc_AttributeError,
-                        "_fields_ is final");
-        return -1;
-    }
-    STGINFO_LOCK(stginfo);
-    // check again after locking
     if (stginfo_get_dict_final(stginfo) == 1) {/* is final ? */
         PyErr_SetString(PyExc_AttributeError,
                         "_fields_ is final");