Skip to content

Commit 92072bd

Browse files
greateggsgreggreateggsgreg
committed
Wider thread-safety audit fixes for free-threaded Python
Audit found several more shared-state hazards beyond the registries already covered. The following are reachable on every interpreter under sufficient contention; FT exposes them reliably. src/runtime/DelegateManager.cs Lock cache lookup + Reflection.Emit. TypeBuilder/ModuleBuilder are not thread-safe; concurrent Python->CLR delegate construction (e.g. PythonEngine.ShutdownHandler(lambda: None) from multiple threads) threw "Duplicate type name within an assembly" on 3.14t. Same shape as the CreateDerivedType fix from a18e872. src/runtime/Finalizer.cs - `_throttled` becomes Interlocked.Increment / Interlocked.Exchange. Lost increments would either grow the queue unbounded or burn CPU on unnecessary drains. - `started` is now volatile so the ThrottledCollect check on every PyObject ctor cannot observe a stale "false" after Initialize. src/runtime/PythonTypes/PyBuffer.cs `disposedValue` is now an int gated by Interlocked.Exchange (write) and Volatile.Read (hot reads). The .NET finalizer racing with an explicit Dispose() could otherwise both pass the `if (!disposedValue)` check and call PyBuffer_Release twice -> double-free of _view.obj. Repeated check at every public method extracted to ThrowIfDisposed(). src/runtime/Util/GenericUtil.cs `mapping` (nested Dictionary<string, Dictionary<string, List<string>>>) guarded by a lock; nested mutations cannot be expressed atomically with ConcurrentDictionary alone. GenericByName snapshots candidate names under the lock then calls AssemblyManager.LookupTypes outside it, since LookupTypes can re-enter Register. src/runtime/PythonEngine.cs - `ShutdownHandlers` (List<>) wrapped in a lock. ConcurrentStack would change semantics (no remove-by-equality). ExecuteShutdownHandlers pops under the lock and invokes unlocked so handlers can re-enter Add/Remove without deadlock. - `initialized` flag is now volatile (read from worker threads, written from Initialize/Shutdown). src/runtime/Runtime.cs - `run` epoch now Interlocked.Increment + Volatile.Read; lost increments across re-init would let stale finalizer queue entries slip past the RuntimeRun guard. - `_pyRefs` mutations wrapped in a lock; ResetPyMembers snapshots then disposes outside the lock so a Dispose callback cannot reenter and deadlock. - `_isInitialized`, `_typesInitialized` now volatile. tests/test_thread.py - test_concurrent_delegate_creation (FT-only): reproduces the DelegateManager Reflection.Emit race - aborts with the lock removed, passes with it. - test_concurrent_shutdown_handler_register (FT-only): drives AddShutdownHandler/RemoveShutdownHandler from 8 threads on pre-built handlers. - Removed test_freethreaded_concurrent_attribute_access_no_tear; its workload duplicates test_concurrent_attribute_access at a different intensity without exercising additional code paths. Comment cleanup Trimmed multi-line thread-safety comments across the branch's earlier commits to single lines that capture only the non-obvious "why" (Concurrent: / Lock: / volatile: / Atomic claim:). Removed comments where the type signature already documents the choice.
1 parent c02ff70 commit 92072bd

17 files changed

Lines changed: 178 additions & 153 deletions

‎src/runtime/ClassManager.cs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,9 @@ internal class ClassManager
3434
BindingFlags.Public |
3535
BindingFlags.NonPublic;
3636

37-
// Two-cache design for thread-safe type creation:
38-
// `cache` — only fully-initialised types; safe to read without the lock.
39-
// `_inProgressCache` — partial types being built inside the lock; visible only
40-
// to the building thread (for self-referential definitions).
37+
// cache: fully-initialised types (lock-free reads).
38+
// _inProgressCache: partial types visible only to the lock-holding builder
39+
// for self-referential definitions.
4140
internal static ConcurrentDictionary<MaybeType, ReflectedClrType> cache = new();
4241
internal static readonly ConcurrentDictionary<MaybeType, ReflectedClrType> _inProgressCache = new();
4342
internal static readonly object _cacheCreateLock = new();

‎src/runtime/DelegateManager.cs‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ namespace Python.Runtime
1616
internal class DelegateManager
1717
{
1818
private readonly Dictionary<Type,Type> cache = new();
19+
// Reflection.Emit is not thread-safe; serialise cache lookup + DefineType.
20+
private readonly object _emitLock = new();
1921
private readonly Type basetype = typeof(Dispatcher);
2022
private readonly Type arrayType = typeof(object[]);
2123
private readonly Type voidtype = typeof(void);
@@ -37,18 +39,14 @@ public DelegateManager()
3739
/// </summary>
3840
private Type GetDispatcher(Type dtype)
3941
{
40-
// If a dispatcher type for the given delegate type has already
41-
// been generated, get it from the cache. The cache maps delegate
42-
// types to generated dispatcher types. A possible optimization
43-
// for the future would be to generate dispatcher types based on
44-
// unique signatures rather than delegate types, since multiple
45-
// delegate types with the same sig could use the same dispatcher.
46-
47-
if (cache.TryGetValue(dtype, out Type item))
42+
lock (_emitLock)
4843
{
49-
return item;
44+
return cache.TryGetValue(dtype, out Type item) ? item : BuildDispatcher(dtype);
5045
}
46+
}
5147

48+
private Type BuildDispatcher(Type dtype)
49+
{
5250
string name = $"__{dtype.FullName}Dispatcher";
5351
name = name.Replace('.', '_');
5452
name = name.Replace('+', '_');

‎src/runtime/Finalizer.cs‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ public ErrorArgs(Exception error)
3636
[DefaultValue(DefaultThreshold)]
3737
public int Threshold { get; set; } = DefaultThreshold;
3838

39-
bool started;
39+
// volatile: ThrottledCollect on PyObject ctor races with Initialize.
40+
volatile bool started;
4041

4142
[DefaultValue(true)]
4243
public bool Enable { get; set; } = true;
@@ -113,13 +114,10 @@ internal void ThrottledCollect()
113114
{
114115
if (!started) throw new InvalidOperationException($"{nameof(PythonEngine)} is not initialized");
115116

116-
_throttled = unchecked(this._throttled + 1);
117-
if (!started || !Enable || _throttled < Threshold) return;
118-
// Skip the drain while Python is finalizing: the .NET-side queue may
119-
// contain references that became stale during teardown, and calling
120-
// Py_DecRef on torn-down state segfaults under free-threaded Python.
117+
if (!Enable || Interlocked.Increment(ref _throttled) < Threshold) return;
118+
// Stale pointers on the queue would crash Py_DecRef during teardown.
121119
if (Runtime._Py_IsFinalizing() == true) return;
122-
_throttled = 0;
120+
Interlocked.Exchange(ref _throttled, 0);
123121
this.Collect();
124122
}
125123

@@ -140,9 +138,7 @@ internal void AddFinalizedObject(ref IntPtr obj, int run
140138
return;
141139
}
142140

143-
// Skip the Refcount sanity check under free-threaded Python: the .NET
144-
// finalizer thread can race with Py_Finalize and a stale read of
145-
// ob_ref_local will crash the process. Keep the check on GIL builds.
141+
// Skip on FT: stale ob_ref_local read from the finalizer thread can crash.
146142
if (!Native.ABI.IsFreeThreaded)
147143
{
148144
Debug.Assert(Runtime.Refcount(new BorrowedReference(obj)) > 0);

‎src/runtime/Interop.cs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,7 @@ public enum TypeFlags: long
105105

106106
internal class Interop
107107
{
108-
// Hit on every type-slot installation; thread-safe for free-threaded Python.
109-
// Two threads racing past TryGetValue with the old plain Dictionary would
110-
// throw on Add ("duplicate key") instead of silently picking one winner.
108+
// Concurrent: type-slot installation can race past TryGetValue.
111109
static readonly ConcurrentDictionary<MethodInfo, Type> delegateTypes = new();
112110

113111
internal static Type GetPrototype(MethodInfo method)

‎src/runtime/Native/ABI.cs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,10 @@ namespace Python.Runtime.Native
77

88
static class ABI
99
{
10-
// Offset of ob_refcnt within PyObject. GIL builds only; on free-threaded
11-
// Python the refcount is split (ob_ref_local + ob_ref_shared) and Refcount
12-
// uses Py_REFCNT instead.
10+
// GIL builds only. FT splits the refcount; Refcount uses Py_REFCNT.
1311
public static int RefCountOffset { get; private set; }
1412

15-
// Bytes to add to generated TypeOffset values for absolute PyHeapTypeObject
16-
// offsets. Free-threaded PyObject_HEAD is 16 bytes larger than the GIL one.
13+
// Added to generated TypeOffsets. FT PyObject_HEAD is 16 bytes larger.
1714
public static int ObjectHeadOffset { get; private set; }
1815

1916
public static bool IsFreeThreaded { get; private set; }

‎src/runtime/PythonEngine.cs‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ namespace Python.Runtime
1616
public class PythonEngine : IDisposable
1717
{
1818
private static DelegateManager? delegateManager;
19-
private static bool initialized;
19+
// volatile: read from worker threads, written from Initialize/Shutdown.
20+
private static volatile bool initialized;
2021
private static IntPtr _pythonHome = IntPtr.Zero;
2122
private static IntPtr _programName = IntPtr.Zero;
2223
private static IntPtr _pythonPath = IntPtr.Zero;
@@ -405,7 +406,9 @@ public static void Shutdown()
405406
/// </summary>
406407
public delegate void ShutdownHandler();
407408

409+
// Lock: ConcurrentStack lacks remove-by-equality; List<> needs serialised mutation.
408410
static readonly List<ShutdownHandler> ShutdownHandlers = new();
411+
static readonly object _shutdownHandlersLock = new();
409412

410413
/// <summary>
411414
/// Add a function to be called when the engine is shut down.
@@ -422,7 +425,7 @@ public static void Shutdown()
422425
/// </summary>
423426
public static void AddShutdownHandler(ShutdownHandler handler)
424427
{
425-
ShutdownHandlers.Add(handler);
428+
lock (_shutdownHandlersLock) ShutdownHandlers.Add(handler);
426429
}
427430

428431
/// <summary>
@@ -435,12 +438,15 @@ public static void AddShutdownHandler(ShutdownHandler handler)
435438
/// </summary>
436439
public static void RemoveShutdownHandler(ShutdownHandler handler)
437440
{
438-
for (int index = ShutdownHandlers.Count - 1; index >= 0; --index)
441+
lock (_shutdownHandlersLock)
439442
{
440-
if (ShutdownHandlers[index] == handler)
443+
for (int index = ShutdownHandlers.Count - 1; index >= 0; --index)
441444
{
442-
ShutdownHandlers.RemoveAt(index);
443-
break;
445+
if (ShutdownHandlers[index] == handler)
446+
{
447+
ShutdownHandlers.RemoveAt(index);
448+
break;
449+
}
444450
}
445451
}
446452
}
@@ -452,10 +458,17 @@ public static void RemoveShutdownHandler(ShutdownHandler handler)
452458
/// </summary>
453459
static void ExecuteShutdownHandlers()
454460
{
455-
while(ShutdownHandlers.Count > 0)
461+
// Invoke unlocked so handlers can re-enter Add/Remove.
462+
while (true)
456463
{
457-
var handler = ShutdownHandlers[ShutdownHandlers.Count - 1];
458-
ShutdownHandlers.RemoveAt(ShutdownHandlers.Count - 1);
464+
ShutdownHandler handler;
465+
lock (_shutdownHandlersLock)
466+
{
467+
if (ShutdownHandlers.Count == 0) return;
468+
int last = ShutdownHandlers.Count - 1;
469+
handler = ShutdownHandlers[last];
470+
ShutdownHandlers.RemoveAt(last);
471+
}
459472
handler();
460473
}
461474
}

‎src/runtime/PythonTypes/PyBuffer.cs‎

Lines changed: 25 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using System.Diagnostics;
44
using System.Linq;
55
using System.Runtime.InteropServices;
6+
using System.Threading;
67

78
namespace Python.Runtime
89
{
@@ -100,8 +101,7 @@ public static long SizeFromFormat(string format)
100101
/// <param name="order">C-style (order is 'C') or Fortran-style (order is 'F') contiguous or either one (order is 'A')</param>
101102
public bool IsContiguous(BufferOrderStyle order)
102103
{
103-
if (disposedValue)
104-
throw new ObjectDisposedException(nameof(PyBuffer));
104+
ThrowIfDisposed();
105105
return Convert.ToBoolean(Runtime.PyBuffer_IsContiguous(ref _view, OrderStyleToChar(order, true)));
106106
}
107107

@@ -111,8 +111,7 @@ public bool IsContiguous(BufferOrderStyle order)
111111
public IntPtr GetPointer(long[] indices)
112112
{
113113
if (indices is null) throw new ArgumentNullException(nameof(indices));
114-
if (disposedValue)
115-
throw new ObjectDisposedException(nameof(PyBuffer));
114+
ThrowIfDisposed();
116115
if (Runtime.PyVersion < new Version(3, 7))
117116
throw new NotSupportedException("GetPointer requires at least Python 3.7");
118117
return Runtime.PyBuffer_GetPointer(ref _view, indices.Select(x => checked((nint)x)).ToArray());
@@ -123,8 +122,7 @@ public IntPtr GetPointer(long[] indices)
123122
/// </summary>
124123
public void FromContiguous(IntPtr buf, long len, BufferOrderStyle fort)
125124
{
126-
if (disposedValue)
127-
throw new ObjectDisposedException(nameof(PyBuffer));
125+
ThrowIfDisposed();
128126
if (Runtime.PyVersion < new Version(3, 7))
129127
throw new NotSupportedException("FromContiguous requires at least Python 3.7");
130128

@@ -139,8 +137,7 @@ public void FromContiguous(IntPtr buf, long len, BufferOrderStyle fort)
139137
/// <param name="buf">Buffer to copy to</param>
140138
public void ToContiguous(IntPtr buf, BufferOrderStyle order)
141139
{
142-
if (disposedValue)
143-
throw new ObjectDisposedException(nameof(PyBuffer));
140+
ThrowIfDisposed();
144141

145142
if (Runtime.PyBuffer_ToContiguous(buf, ref _view, _view.len, OrderStyleToChar(order, true)) < 0)
146143
throw PythonException.ThrowLastAsClrException();
@@ -166,8 +163,7 @@ internal static void FillContiguousStrides(int ndims, IntPtr shape, IntPtr strid
166163
/// <returns>On success, set view->obj to a new reference to exporter and return 0. Otherwise, raise PyExc_BufferError, set view->obj to NULL and return -1;</returns>
167164
internal void FillInfo(BorrowedReference exporter, IntPtr buf, long len, bool _readonly, int flags)
168165
{
169-
if (disposedValue)
170-
throw new ObjectDisposedException(nameof(PyBuffer));
166+
ThrowIfDisposed();
171167
if (Runtime.PyBuffer_FillInfo(ref _view, exporter, buf, (IntPtr)len, Convert.ToInt32(_readonly), flags) < 0)
172168
throw PythonException.ThrowLastAsClrException();
173169
}
@@ -177,8 +173,7 @@ internal void FillInfo(BorrowedReference exporter, IntPtr buf, long len, bool _r
177173
/// </summary>
178174
public void Write(byte[] buffer, int sourceOffset, int count, nint destinationOffset)
179175
{
180-
if (disposedValue)
181-
throw new ObjectDisposedException(nameof(PyBuffer));
176+
ThrowIfDisposed();
182177
if (_view.ndim != 1)
183178
throw new NotImplementedException("Multidimensional arrays, scalars and objects without a buffer are not supported.");
184179
if (!this.IsContiguous(BufferOrderStyle.C))
@@ -207,8 +202,7 @@ public void Write(byte[] buffer, int sourceOffset, int count, nint destinationOf
207202
/// Reads the buffer of a python object into a managed byte array. This can be used to pass data like images from python to managed.
208203
/// </summary>
209204
public void Read(byte[] buffer, int destinationOffset, int count, nint sourceOffset) {
210-
if (disposedValue)
211-
throw new ObjectDisposedException(nameof(PyBuffer));
205+
ThrowIfDisposed();
212206
if (_view.ndim != 1)
213207
throw new NotImplementedException("Multidimensional arrays, scalars and objects without a buffer are not supported.");
214208
if (!this.IsContiguous(BufferOrderStyle.C))
@@ -231,31 +225,32 @@ public void Read(byte[] buffer, int destinationOffset, int count, nint sourceOff
231225
Marshal.Copy(_view.buf + sourceOffset, buffer, destinationOffset, count);
232226
}
233227

234-
private bool disposedValue = false; // To detect redundant calls
228+
// 0/1; atomic so finalizer + Dispose cannot double-free the buffer.
229+
private int disposedValue;
235230

236-
private void Dispose(bool disposing)
231+
private void ThrowIfDisposed()
237232
{
238-
if (!disposedValue)
239-
{
240-
if (Runtime.Py_IsInitialized() == 0)
241-
throw new InvalidOperationException("Python runtime must be initialized");
233+
if (Volatile.Read(ref disposedValue) != 0)
234+
throw new ObjectDisposedException(nameof(PyBuffer));
235+
}
242236

243-
// this also decrements ref count for _view->obj
244-
Runtime.PyBuffer_Release(ref _view);
237+
private void Dispose(bool disposing)
238+
{
239+
if (Interlocked.Exchange(ref disposedValue, 1) != 0) return;
240+
if (Runtime.Py_IsInitialized() == 0)
241+
throw new InvalidOperationException("Python runtime must be initialized");
245242

246-
_exporter = null!;
247-
Shape = null;
248-
Strides = null;
249-
SubOffsets = null;
243+
// this also decrements ref count for _view->obj
244+
Runtime.PyBuffer_Release(ref _view);
250245

251-
disposedValue = true;
252-
}
246+
_exporter = null!;
247+
Shape = null;
248+
Strides = null;
249+
SubOffsets = null;
253250
}
254251

255252
~PyBuffer()
256253
{
257-
Debug.Assert(!disposedValue);
258-
259254
if (_view.obj != IntPtr.Zero)
260255
{
261256
Finalizer.Instance.AddFinalizedBuffer(ref _view);

‎src/runtime/PythonTypes/PyObject.cs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,7 @@ internal PyObject(in StolenReference reference)
104104
CheckRun();
105105
#endif
106106

107-
// If Python is finalizing we cannot safely enqueue a decref:
108-
// the .NET finalizer thread runs concurrently with Py_Finalize
109-
// and any later Py_DecRef would touch torn-down state. Drop
110-
// the reference and let process exit reclaim the memory.
107+
// Drop the reference if Python is tearing down; queued Py_DecRef would crash.
111108
if (Runtime._Py_IsFinalizing() == true)
112109
{
113110
rawPtr = IntPtr.Zero;

0 commit comments

Comments
 (0)