From e1b2f6fe2758dc56a8fe26d9933cd08b943990d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Valentin=20Breu=C3=9F?= Date: Fri, 30 Oct 2026 00:24:09 +0100 Subject: [PATCH] fix: do not hold a lock while matching indexer setups Indexer setups were matched inside the lock of their list, so a slow or blocking matcher stalled every other thread that accessed or set up an indexer on the same mock, and could deadlock when it waited for such a thread. The setups are now copied on write, like the member-id tables, so matching reads them without a lock and without allocating. --- Source/Mockolate/Setup/MockSetups.Indexers.cs | 109 +++++++----------- .../Setup/MockSetupsTests.IndexersTests.cs | 33 ++++++ .../TestHelpers/FakeIndexerSetup.cs | 12 +- 3 files changed, 87 insertions(+), 67 deletions(-) diff --git a/Source/Mockolate/Setup/MockSetups.Indexers.cs b/Source/Mockolate/Setup/MockSetups.Indexers.cs index c0d8706d1..9bf0852e9 100644 --- a/Source/Mockolate/Setup/MockSetups.Indexers.cs +++ b/Source/Mockolate/Setup/MockSetups.Indexers.cs @@ -17,26 +17,12 @@ internal partial class MockSetups #endif internal sealed class IndexerSetups { - private List? _storage; + private IndexerSetup[]? _storage; + private readonly MockolateLock _storageLock = new(); private IndexerValueStorage?[]? _valueStorages; private readonly object _valueStoragesLock = new(); - public int Count - { - get - { - List? storage = _storage; - if (storage is null) - { - return 0; - } - - lock (storage) - { - return storage.Count; - } - } - } + public int Count => Volatile.Read(ref _storage)?.Length ?? 0; /// /// Pre-sizes the per-signature value-storage array. Optional — @@ -110,32 +96,19 @@ internal IndexerValueStorage GetOrCreateStorage(int signatureInd } } - private List GetOrCreateStorage() - { - if (_storage is null) - { - Interlocked.CompareExchange(ref _storage, [], null); - } - - return _storage!; - } - public T? GetMatching(Func predicate) where T : IndexerSetup { - List? storage = _storage; + IndexerSetup[]? storage = Volatile.Read(ref _storage); if (storage is null) { return null; } - lock (storage) + for (int i = storage.Length - 1; i >= 0; i--) { - for (int i = storage.Count - 1; i >= 0; i--) + if (storage[i] is T typedSetup && predicate(typedSetup)) { - if (storage[i] is T typedSetup && predicate(typedSetup)) - { - return typedSetup; - } + return typedSetup; } } @@ -144,21 +117,18 @@ private List GetOrCreateStorage() public T? GetMatching(IndexerAccess access) where T : IndexerSetup { - List? storage = _storage; + IndexerSetup[]? storage = Volatile.Read(ref _storage); if (storage is null) { return null; } - lock (storage) + for (int i = storage.Length - 1; i >= 0; i--) { - for (int i = storage.Count - 1; i >= 0; i--) + if (storage[i] is T typedSetup && + ((IInteractiveIndexerSetup)typedSetup).Matches(access)) { - if (storage[i] is T typedSetup && - ((IInteractiveIndexerSetup)typedSetup).Matches(access)) - { - return typedSetup; - } + return typedSetup; } } @@ -169,50 +139,59 @@ private List GetOrCreateStorage() [ExcludeFromCodeCoverage] public override string ToString() { - List? storage = _storage; - if (storage is null || storage.Count == 0) + IndexerSetup[]? storage = Volatile.Read(ref _storage); + if (storage is null || storage.Length == 0) { return "0 indexers"; } - lock (storage) + int count = storage.Length; + StringBuilder sb = new(); + sb.Append(count).Append(count == 1 ? " indexer:" : " indexers:").AppendLine(); + foreach (IndexerSetup indexerSetup in storage) { - int count = storage.Count; - StringBuilder sb = new(); - sb.Append(count).Append(count == 1 ? " indexer:" : " indexers:").AppendLine(); - foreach (IndexerSetup indexerSetup in storage) - { - sb.Append(indexerSetup).AppendLine(); - } - - sb.Length -= Environment.NewLine.Length; - return sb.ToString(); + sb.Append(indexerSetup).AppendLine(); } + + sb.Length -= Environment.NewLine.Length; + return sb.ToString(); } + /// + /// Copies the setups on write, so that matching reads them without a lock while it runs user code. + /// internal void Add(IndexerSetup setup) { - List storage = GetOrCreateStorage(); - lock (storage) + lock (_storageLock) { - storage.Add(setup); + IndexerSetup[]? existing = _storage; + IndexerSetup[] storage; + if (existing is null) + { + storage = [setup,]; + } + else + { + storage = new IndexerSetup[existing.Length + 1]; + Array.Copy(existing, storage, existing.Length); + storage[existing.Length] = setup; + } + + Volatile.Write(ref _storage, storage); } } internal IEnumerable EnumerateUnusedSetupsBy(IMockInteractions interactions) { - List? storage = _storage; + IndexerSetup[]? storage = Volatile.Read(ref _storage); if (storage is null) { return []; } - lock (storage) - { - return storage.Where(indexerSetup => interactions.OfType() - .All(indexerAccess => !((IInteractiveIndexerSetup)indexerSetup).Matches(indexerAccess))) - .ToList(); - } + return storage.Where(indexerSetup => interactions.OfType() + .All(indexerAccess => !((IInteractiveIndexerSetup)indexerSetup).Matches(indexerAccess))) + .ToList(); } } } diff --git a/Tests/Mockolate.Internal.Tests/Setup/MockSetupsTests.IndexersTests.cs b/Tests/Mockolate.Internal.Tests/Setup/MockSetupsTests.IndexersTests.cs index 024a6a56a..6d5f89d25 100644 --- a/Tests/Mockolate.Internal.Tests/Setup/MockSetupsTests.IndexersTests.cs +++ b/Tests/Mockolate.Internal.Tests/Setup/MockSetupsTests.IndexersTests.cs @@ -34,6 +34,36 @@ public async Task GetMatching_OnEmptyStorage_ShouldReturnNullWithoutThrowing() await That(result).IsNull(); } + [Test] + public async Task GetMatching_WithAccess_ShouldNotBlockOtherThreadsWhileMatching() + { + MockSetups.IndexerSetups setups = new(); + bool addedWhileMatching = false; + setups.Add(new FakeIndexerSetup(true, () => addedWhileMatching = AddFromOtherThread(setups))); + + _ = setups.GetMatching(new FakeIndexerAccess()); + + await That(addedWhileMatching).IsTrue() + .Because("a matcher runs user code, so it must not hold the lock that other threads need to access the setups"); + } + + [Test] + public async Task GetMatching_WithPredicate_ShouldNotBlockOtherThreadsWhileMatching() + { + MockSetups.IndexerSetups setups = new(); + setups.Add(new FakeIndexerSetup(true)); + bool addedWhileMatching = false; + + _ = setups.GetMatching(_ => + { + addedWhileMatching = AddFromOtherThread(setups); + return true; + }); + + await That(addedWhileMatching).IsTrue() + .Because("the predicate runs user code, so it must not hold the lock that other threads need to access the setups"); + } + [Test] public async Task InitializeStorageCount_OnFreshInstance_ShouldAllocateValueStoragesArrayWithRequestedLength() { @@ -108,6 +138,9 @@ public async Task ThreadSafety_ShouldHandleParallelAdds() await That(setups.Count).IsEqualTo(200); } + private static bool AddFromOtherThread(MockSetups.IndexerSetups setups) + => Task.Run(() => setups.Add(new FakeIndexerSetup(false))).Wait(TimeSpan.FromSeconds(5)); + private static Array? GetValueStoragesField(MockSetups.IndexerSetups setups) { FieldInfo field = typeof(MockSetups.IndexerSetups).GetField( diff --git a/Tests/Mockolate.Internal.Tests/TestHelpers/FakeIndexerSetup.cs b/Tests/Mockolate.Internal.Tests/TestHelpers/FakeIndexerSetup.cs index 6821fb305..680f8c98c 100644 --- a/Tests/Mockolate.Internal.Tests/TestHelpers/FakeIndexerSetup.cs +++ b/Tests/Mockolate.Internal.Tests/TestHelpers/FakeIndexerSetup.cs @@ -5,9 +5,13 @@ namespace Mockolate.Internal.Tests.TestHelpers; internal sealed class FakeIndexerSetup : IndexerSetup { - internal FakeIndexerSetup(bool match) : base(new MockRegistry(MockBehavior.Default, new FastMockInteractions(0))) + private readonly Action? _onMatch; + + internal FakeIndexerSetup(bool match, Action? onMatch = null) + : base(new MockRegistry(MockBehavior.Default, new FastMockInteractions(0))) { ShouldMatch = match; + _onMatch = onMatch; } internal bool ShouldMatch { get; } @@ -15,7 +19,11 @@ internal sealed class FakeIndexerSetup : IndexerSetup public static bool InvokeTryCast(object? value, out T result, MockBehavior behavior) => TryCast(value, out result, behavior); - protected override bool MatchesAccess(IndexerAccess access) => ShouldMatch; + protected override bool MatchesAccess(IndexerAccess access) + { + _onMatch?.Invoke(); + return ShouldMatch; + } public override bool? SkipBaseClass() => null;