Skip to content

Commit 0a9d482

Browse files
greateggsgreggreateggsgreg
committed
Make additional internal registries thread-safe
The atomic-type-creation commit (2f08c98) covered the two highest- contention caches. Wider audit found more plain Dictionary / HashSet collections on hot paths that tear under free-threaded Python and also fire Debug.Assert on GIL builds at sufficient contention. Convert them to ConcurrentDictionary: - ModuleObject.cache and ModuleObject.allNames - hit on every Module.Attr access; the old HashSet.Add for allNames could miss add-once semantics under contention and the Dictionary.Remove on teardown could observe a torn map. - Interop.delegateTypes - racing past TryGetValue with the old plain Dictionary threw on Add ("duplicate key") instead of silently picking one winner, which became reproducible on high concurrency. - ClassBase.ClearVisited - re-entrancy guard for tp_clear, hit from both the main thread and the .NET finalizer thread; the plain HashSet tore reliably under FT. Also tidy the existing ClassManager / TypeManager / ExtensionType ConcurrentDictionary references via a using-directive instead of fully qualifying System.Collections.Concurrent at every site.
1 parent a18e872 commit 0a9d482

6 files changed

Lines changed: 26 additions & 14 deletions

File tree

‎src/runtime/ClassManager.cs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Concurrent;
23
using System.Collections.Generic;
34
using System.Diagnostics;
45
using System.Linq;
@@ -37,8 +38,8 @@ internal class ClassManager
3738
// `cache` — only fully-initialised types; safe to read without the lock.
3839
// `_inProgressCache` — partial types being built inside the lock; visible only
3940
// to the building thread (for self-referential definitions).
40-
internal static System.Collections.Concurrent.ConcurrentDictionary<MaybeType, ReflectedClrType> cache = new();
41-
internal static readonly System.Collections.Concurrent.ConcurrentDictionary<MaybeType, ReflectedClrType> _inProgressCache = new();
41+
internal static ConcurrentDictionary<MaybeType, ReflectedClrType> cache = new();
42+
internal static readonly ConcurrentDictionary<MaybeType, ReflectedClrType> _inProgressCache = new();
4243
internal static readonly object _cacheCreateLock = new();
4344
private static readonly Type dtype;
4445

@@ -115,7 +116,7 @@ internal static ClassManagerState SaveRuntimeData()
115116

116117
internal static void RestoreRuntimeData(ClassManagerState storage)
117118
{
118-
cache = new System.Collections.Concurrent.ConcurrentDictionary<MaybeType, ReflectedClrType>(storage.Cache);
119+
cache = new ConcurrentDictionary<MaybeType, ReflectedClrType>(storage.Cache);
119120
var invalidClasses = new List<KeyValuePair<MaybeType, ReflectedClrType>>();
120121
var contexts = storage.Contexts;
121122
foreach (var pair in cache)

‎src/runtime/Interop.cs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Concurrent;
23
using System.Collections.Generic;
34
using System.Diagnostics;
45
using System.Linq;
@@ -104,7 +105,10 @@ public enum TypeFlags: long
104105

105106
internal class Interop
106107
{
107-
static readonly Dictionary<MethodInfo, Type> delegateTypes = new();
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.
111+
static readonly ConcurrentDictionary<MethodInfo, Type> delegateTypes = new();
108112

109113
internal static Type GetPrototype(MethodInfo method)
110114
{
@@ -131,7 +135,7 @@ internal static Type GetPrototype(MethodInfo method)
131135

132136
if (invoke.ReturnType != method.ReturnType) continue;
133137

134-
delegateTypes.Add(method, candidate);
138+
delegateTypes.TryAdd(method, candidate);
135139
return candidate;
136140
}
137141

‎src/runtime/TypeManager.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Concurrent;
23
using System.Collections.Generic;
34
using System.Linq;
45
using System.Reflection;
@@ -27,7 +28,7 @@ internal class TypeManager
2728

2829
private const BindingFlags tbFlags = BindingFlags.Public | BindingFlags.Static;
2930
// Thread-safe cache; multi-step creation in GetType is serialised via _cacheCreateLock.
30-
internal static readonly System.Collections.Concurrent.ConcurrentDictionary<MaybeType, PyType> cache = new();
31+
internal static readonly ConcurrentDictionary<MaybeType, PyType> cache = new();
3132
internal static readonly object _cacheCreateLock = new();
3233

3334
static readonly Dictionary<PyType, SlotsHolder> _slotsHolders = new(PythonReferenceComparer.Instance);

‎src/runtime/Types/ClassBase.cs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using System;
22
using System.Collections;
3+
using System.Collections.Concurrent;
34
using System.Collections.Generic;
45
using System.Diagnostics;
56
using System.Linq;
@@ -374,7 +375,9 @@ public static int tp_clear(BorrowedReference ob)
374375
return 0;
375376
}
376377

377-
static readonly HashSet<IntPtr> ClearVisited = new();
378+
// Re-entrancy guard for tp_clear. Thread-safe so concurrent tp_clear from
379+
// different threads (free-threaded or finalizer-thread) cannot tear the set.
380+
static readonly ConcurrentDictionary<IntPtr, byte> ClearVisited = new();
378381

379382
internal static unsafe int BaseUnmanagedClear(BorrowedReference ob)
380383
{
@@ -390,11 +393,11 @@ internal static unsafe int BaseUnmanagedClear(BorrowedReference ob)
390393
if (clearPtr == TypeManager.subtype_clear)
391394
{
392395
var addr = ob.DangerousGetAddress();
393-
if (!ClearVisited.Add(addr))
396+
if (!ClearVisited.TryAdd(addr, 0))
394397
return 0;
395398

396399
int res = clear(ob);
397-
ClearVisited.Remove(addr);
400+
ClearVisited.TryRemove(addr, out _);
398401
return res;
399402
}
400403
else

‎src/runtime/Types/ExtensionType.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Concurrent;
23
using System.Collections.Generic;
34
using System.Diagnostics;
45
using System.Runtime.InteropServices;
@@ -44,7 +45,7 @@ public virtual NewReference Alloc()
4445

4546
// "borrowed" references; thread-safe to survive free-threaded Python and
4647
// .NET finalizer-thread races.
47-
internal static readonly System.Collections.Concurrent.ConcurrentDictionary<IntPtr, byte> loadedExtensions = new();
48+
internal static readonly ConcurrentDictionary<IntPtr, byte> loadedExtensions = new();
4849
void SetupGc (BorrowedReference ob, BorrowedReference tp)
4950
{
5051
GCHandle gc = GCHandle.Alloc(this);

‎src/runtime/Types/ModuleObject.cs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Concurrent;
23
using System.Collections.Generic;
34
using System.Diagnostics;
45
using System.Linq;
@@ -13,13 +14,14 @@ namespace Python.Runtime
1314
[Serializable]
1415
internal class ModuleObject : ExtensionType
1516
{
16-
private readonly Dictionary<string, PyObject> cache = new();
17+
// Hot path on every `Module.Attr` access; thread-safe for free-threaded Python.
18+
private readonly ConcurrentDictionary<string, PyObject> cache = new();
1719

1820
internal string moduleName;
1921
internal PyDict dict;
2022
protected string _namespace;
2123
private readonly PyList __all__ = new ();
22-
private readonly HashSet<string> allNames = new();
24+
private readonly ConcurrentDictionary<string, byte> allNames = new();
2325

2426
// Attributes to be set on the module according to PEP302 and 451
2527
// by the import machinery.
@@ -193,7 +195,7 @@ public void LoadNames()
193195
hasValidAttribute = !attrVal.IsNull();
194196
}
195197

196-
if (hasValidAttribute && allNames.Add(name))
198+
if (hasValidAttribute && allNames.TryAdd(name, 0))
197199
{
198200
// if it's a valid attribute, add it to __all__ once.
199201
using var pyname = Runtime.PyString_FromString(name);
@@ -267,7 +269,7 @@ internal void ResetModuleMembers()
267269
}
268270
Runtime.PyErr_Clear();
269271
}
270-
cache.Remove(memberName);
272+
cache.TryRemove(memberName, out _);
271273
}
272274
}
273275

0 commit comments

Comments
 (0)