Skip to content

Commit 0f24283

Browse files
greateggsgreggreateggsgreg
committed
Make extension/CLR-object registries thread-safe
ExtensionType.loadedExtensions and CLRObject.reflectedObjects are "borrowed reference" registries written on every alloc and read or removed from finalizer-thread paths. Under free-threaded Python the plain HashSet<IntPtr> tears reliably; under the GIL the same tears were happening more rarely but still mostly observable as Debug.Assert firings during shutdown. Convert both to ConcurrentDictionary<IntPtr, byte> with the equivalent TryAdd/TryRemove operations, and update the few non-mutating callers (NullGCHandles, RuntimeData snapshot LINQ) to enumerate Keys.
1 parent c1a9aaa commit 0f24283

4 files changed

Lines changed: 13 additions & 11 deletions

File tree

‎src/runtime/StateSerialization/RuntimeData.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ private static SharedObjectsState SaveRuntimeDataObjects()
178178
var contexts = new Dictionary<PyObject, Dictionary<string, object?>>(PythonReferenceComparer.Instance);
179179
var extensionObjs = new Dictionary<PyObject, ExtensionType>(PythonReferenceComparer.Instance);
180180
// make a copy with strongly typed references to avoid concurrent modification
181-
var extensions = ExtensionType.loadedExtensions
181+
var extensions = ExtensionType.loadedExtensions.Keys
182182
.Select(addr => new PyObject(
183183
new BorrowedReference(addr),
184184
// if we don't skip collect, finalizer might modify loadedExtensions
@@ -199,7 +199,7 @@ private static SharedObjectsState SaveRuntimeDataObjects()
199199
var wrappers = new Dictionary<object, List<CLRObject>>();
200200
var userObjects = new CLRWrapperCollection();
201201
// make a copy with strongly typed references to avoid concurrent modification
202-
var reflectedObjects = CLRObject.reflectedObjects
202+
var reflectedObjects = CLRObject.reflectedObjects.Keys
203203
.Select(addr => new PyObject(
204204
new BorrowedReference(addr),
205205
// if we don't skip collect, finalizer might modify reflectedObjects

‎src/runtime/Types/ClassBase.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ public static int tp_clear(BorrowedReference ob)
360360
if (TryFreeGCHandle(ob))
361361
{
362362
IntPtr addr = ob.DangerousGetAddress();
363-
bool deleted = CLRObject.reflectedObjects.Remove(addr);
363+
bool deleted = CLRObject.reflectedObjects.TryRemove(addr, out _);
364364
Debug.Assert(deleted);
365365
}
366366

‎src/runtime/Types/ClrObject.cs‎

Lines changed: 5 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.Runtime.InteropServices;
@@ -13,8 +14,8 @@ internal sealed class CLRObject : ManagedType
1314

1415
internal static bool creationBlocked = false;
1516

16-
// "borrowed" references
17-
internal static readonly HashSet<IntPtr> reflectedObjects = new();
17+
// "borrowed" references; thread-safe (see ExtensionType.loadedExtensions).
18+
internal static readonly ConcurrentDictionary<IntPtr, byte> reflectedObjects = new();
1819
static NewReference Create(object ob, BorrowedReference tp)
1920
{
2021
if (creationBlocked)
@@ -28,7 +29,7 @@ static NewReference Create(object ob, BorrowedReference tp)
2829
GCHandle gc = GCHandle.Alloc(self);
2930
InitGCHandle(py.Borrow(), type: tp, gc);
3031

31-
bool isNew = reflectedObjects.Add(py.DangerousGetAddress());
32+
bool isNew = reflectedObjects.TryAdd(py.DangerousGetAddress(), 0);
3233
Debug.Assert(isNew);
3334

3435
// Fix the BaseException args (and __cause__ in case of Python 3)
@@ -73,7 +74,7 @@ protected override void OnLoad(BorrowedReference ob, Dictionary<string, object?>
7374
GCHandle gc = GCHandle.Alloc(this);
7475
SetGCHandle(ob, gc);
7576

76-
bool isNew = reflectedObjects.Add(ob.DangerousGetAddress());
77+
bool isNew = reflectedObjects.TryAdd(ob.DangerousGetAddress(), 0);
7778
Debug.Assert(isNew);
7879
}
7980
}

‎src/runtime/Types/ExtensionType.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,15 @@ public virtual NewReference Alloc()
4242

4343
public PyObject AllocObject() => new(Alloc().Steal());
4444

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

52-
bool isNew = loadedExtensions.Add(ob.DangerousGetAddress());
53+
bool isNew = loadedExtensions.TryAdd(ob.DangerousGetAddress(), 0);
5354
Debug.Assert(isNew);
5455

5556
// We have to support gc because the type machinery makes it very
@@ -94,7 +95,7 @@ public static int tp_clear(BorrowedReference ob)
9495

9596
if (TryFreeGCHandle(ob))
9697
{
97-
bool deleted = loadedExtensions.Remove(ob.DangerousGetAddress());
98+
bool deleted = loadedExtensions.TryRemove(ob.DangerousGetAddress(), out _);
9899
Debug.Assert(deleted);
99100
}
100101

0 commit comments

Comments
 (0)