Document lock acquisition sites and strong->weak GCHandle swap

greateggsgreg · greateggsgreg · commit 0860ade05e0c · 2026-05-09T22:46:12.000-04:00
Comment-only changes. Adds inline notes at lock acquisitions where the
"why" is not obvious from the field declaration alone:

- ClassDerivedObject.Reset / GetModuleBuilder: explain that both
  builder caches must update atomically and that DefineDynamicAssembly
  / DefineDynamicModule produce duplicates under contention.
- TypeManager.GetType: note that CreateType + cache write must be
  atomic.
- ReflectedClrType.GetOrCreate: cross-file lock; mention it also
  serialises ClassManager.cache and TypeManager._slotsHolders writes.
- Runtime.ResetPyMembers: explain the snapshot-then-dispose pattern
  (Dispose() callbacks would deadlock if invoked under the lock).

Expands the strong-&gt;weak GCHandle swap in ClassDerivedObject.tp_dealloc
to spell out:

1. Why the PyObject is not freed at refcount 0 (C# wrapper may still
   reference it; ToPython() resurrects via _Py_NewReference).
2. Why the handle is demoted to weak (lets the C# wrapper be GC'd; on
   collection PyFinalize enqueues the real PyObject_GC_Del).
3. Why the swap uses Interlocked.Exchange (tp_clear may race on the
   same slot under FT / finalizer thread; without atomic claim both
   threads could observe and double-free the same handle).
diff --git a/src/runtime/Runtime.cs b/src/runtime/Runtime.cs
@@ -414,6 +414,8 @@ private static void SetPyMemberTypeOf(out PyObject obj, StolenReference value)
 
         private static void ResetPyMembers()
         {
+            // Snapshot under lock; Dispose() runs outside it so a callback that
+            // re-enters SetPyMember does not deadlock.
             PyObject[] snapshot;
             lock (_pyRefsLock)
             {
diff --git a/src/runtime/TypeManager.cs b/src/runtime/TypeManager.cs
@@ -99,6 +99,7 @@ internal static PyType GetType(Type type)
         {
             // Cached with refcount 1; effectively lives until the CPython runtime is finalised.
             if (cache.TryGetValue(type, out var pyType)) return pyType;
+            // CreateType + cache write must be atomic so two threads do not both allocate.
             lock (_cacheCreateLock)
             {
                 if (cache.TryGetValue(type, out pyType)) return pyType;
diff --git a/src/runtime/Types/ClassDerived.cs b/src/runtime/Types/ClassDerived.cs
@@ -43,6 +43,8 @@ static ClassDerivedObject()
 
         public static void Reset()
         {
+            // Atomic replacement of both builder caches so a concurrent
+            // GetModuleBuilder cannot observe one new + one old.
             lock (_buildersLock)
             {
                 assemblyBuilders = new Dictionary<string, AssemblyBuilder>();
@@ -83,7 +85,21 @@ protected override void SetTypeNewSlot(BorrowedReference pyType, SlotsHolder slo
             // self may be null after Shutdown begun
             if (self is not null)
             {
-                // Atomically swap strong->weak; concurrent tp_clear may also clear the slot.
+                // Python refcount has reached 0 but we do NOT free the PyObject:
+                // the C# wrapper (via IPythonDerivedType.__pyobj__) may still
+                // reference it, and a later ToPython() on that wrapper will
+                // resurrect a strong handle and call _Py_NewReference.
+                //
+                // Demote the GCHandle from strong -> weak so the C# wrapper can
+                // be collected; when it is, PyFinalize enqueues the actual
+                // PyObject_GC_Del.  Until then the slot keeps the wrapper
+                // reachable from Python -> C# but not the other way.
+                //
+                // Interlocked.Exchange: under FT (or .NET-finalizer-thread
+                // races) tp_clear can hit the same slot.  Whichever thread
+                // observes the original handle frees it; the loser frees the
+                // weak handle it just allocated.  Without the atomic swap both
+                // threads could see and free the same handle (double-free).
                 GCHandle weak = GCHandle.Alloc(self, GCHandleType.Weak);
                 BorrowedReference borrow = ob.Borrow();
                 int offset = Util.ReadInt32(Runtime.PyObject_TYPE(borrow), Offsets.tp_clr_inst_offset);
@@ -678,6 +694,8 @@ private static void AddPythonProperty(string propertyName, PyObject func, TypeBu
         private static ModuleBuilder GetModuleBuilder(string assemblyName, string moduleName)
         {
             var key = Tuple.Create(assemblyName, moduleName);
+            // Cache check-and-create must be atomic; DefineDynamicAssembly /
+            // DefineDynamicModule produce duplicate builders under contention.
             lock (_buildersLock)
             {
                 if (moduleBuilders.TryGetValue(key, out ModuleBuilder? existing))
diff --git a/src/runtime/Types/ReflectedClrType.cs b/src/runtime/Types/ReflectedClrType.cs
@@ -29,6 +29,8 @@ public static ReflectedClrType GetOrCreate(Type type)
         if (ClassManager.cache.TryGetValue(type, out var pyType))
             return pyType;
 
+        // Shared with ClassManager.cache + TypeManager._slotsHolders writes
+        // so the multi-step type build below is atomic.
         lock (ClassManager._cacheCreateLock)
         {
             // Re-check now that we hold the lock; another thread may have finished.

Original file line number	Diff line number	Diff line change
`@@ -414,6 +414,8 @@ private static void SetPyMemberTypeOf(out PyObject obj, StolenReference value)`
`414`	`414`
`415`	`415`	`private static void ResetPyMembers()`
`416`	`416`	`{`
	`417`	`+ // Snapshot under lock; Dispose() runs outside it so a callback that`
	`418`	`+ // re-enters SetPyMember does not deadlock.`
`417`	`419`	`PyObject[] snapshot;`
`418`	`420`	`lock (_pyRefsLock)`
`419`	`421`	`{`
Original file line number	Diff line number	Diff line change
`@@ -99,6 +99,7 @@ internal static PyType GetType(Type type)`
`99`	`99`	`{`
`100`	`100`	`// Cached with refcount 1; effectively lives until the CPython runtime is finalised.`
`101`	`101`	`if (cache.TryGetValue(type, out var pyType)) return pyType;`
	`102`	`+ // CreateType + cache write must be atomic so two threads do not both allocate.`
`102`	`103`	`lock (_cacheCreateLock)`
`103`	`104`	`{`
`104`	`105`	`if (cache.TryGetValue(type, out pyType)) return pyType;`
Original file line number	Diff line number	Diff line change
`@@ -29,6 +29,8 @@ public static ReflectedClrType GetOrCreate(Type type)`
`29`	`29`	`if (ClassManager.cache.TryGetValue(type, out var pyType))`
`30`	`30`	`return pyType;`
`31`	`31`
	`32`	`+ // Shared with ClassManager.cache + TypeManager._slotsHolders writes`
	`33`	`+ // so the multi-step type build below is atomic.`
`32`	`34`	`lock (ClassManager._cacheCreateLock)`
`33`	`35`	`{`
`34`	`36`	`// Re-check now that we hold the lock; another thread may have finished.`