Skip to content

Commit 5326e00

Browse files
rafalmaciagclaude
andcommitted
Add ReaderWriterLockSlim thread safety to ObservableCollection
Fixes IndexOutOfRangeException caused by concurrent access from ConcurrentDictionary.GetOrAdd factories (e.g. PipelinesModel.GetOrCreate). - Write lock on InsertItem, RemoveItem, SetItem, ClearItems, MoveItem - Read lock on BinarySearch - InsertSorted inlines binary search within write lock to avoid deadlock - Per-element read lock enumerator (lock not held across yield) - Index clamping in InsertItem for stale indices from concurrent removes - Added xUnit test project with 14 thread-safety and functional tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent d225a45 commit 5326e00

4 files changed

Lines changed: 493 additions & 39 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
3+
<PropertyGroup>
4+
<TargetFramework>net10.0</TargetFramework>
5+
<ImplicitUsings>enable</ImplicitUsings>
6+
<Nullable>enable</Nullable>
7+
<IsPackable>false</IsPackable>
8+
</PropertyGroup>
9+
10+
<ItemGroup>
11+
<PackageReference Include="coverlet.collector" Version="6.0.4" />
12+
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.14.1" />
13+
<PackageReference Include="xunit" Version="2.9.3" />
14+
<PackageReference Include="xunit.runner.visualstudio" Version="3.1.4" />
15+
</ItemGroup>
16+
17+
<ItemGroup>
18+
<Using Include="Xunit" />
19+
</ItemGroup>
20+
21+
<ItemGroup>
22+
<ProjectReference Include="..\ModelingEvolution.Observable\ModelingEvolution.Observable.csproj" />
23+
</ItemGroup>
24+
25+
</Project>
Lines changed: 298 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,298 @@
1+
using System.Collections.Specialized;
2+
3+
namespace ModelingEvolution.Observable.Tests;
4+
5+
public class ObservableCollectionThreadSafetyTests
6+
{
7+
[Fact]
8+
public void ConcurrentInserts_ShouldNotThrow()
9+
{
10+
var collection = new ObservableCollection<int>();
11+
var exceptions = new List<Exception>();
12+
13+
Parallel.For(0, 1000, i =>
14+
{
15+
try
16+
{
17+
collection.Add(i);
18+
}
19+
catch (Exception ex)
20+
{
21+
lock (exceptions) exceptions.Add(ex);
22+
}
23+
});
24+
25+
Assert.Empty(exceptions);
26+
Assert.Equal(1000, collection.Count);
27+
}
28+
29+
[Fact]
30+
public void ConcurrentInsertSorted_ShouldNotThrow()
31+
{
32+
var collection = new ObservableCollection<int>();
33+
var exceptions = new List<Exception>();
34+
35+
Parallel.For(0, 1000, i =>
36+
{
37+
try
38+
{
39+
collection.InsertSorted(i);
40+
}
41+
catch (Exception ex)
42+
{
43+
lock (exceptions) exceptions.Add(ex);
44+
}
45+
});
46+
47+
Assert.Empty(exceptions);
48+
Assert.Equal(1000, collection.Count);
49+
50+
// Verify sorted order
51+
for (int i = 1; i < collection.Count; i++)
52+
Assert.True(collection[i - 1] <= collection[i],
53+
$"Collection not sorted at index {i}: {collection[i - 1]} > {collection[i]}");
54+
}
55+
56+
[Fact]
57+
public void ConcurrentInsertAndRemove_ShouldNotThrow()
58+
{
59+
var collection = new ObservableCollection<int>();
60+
// Pre-fill so removals have something to work with
61+
for (int i = 0; i < 500; i++) collection.Add(i);
62+
63+
var exceptions = new List<Exception>();
64+
65+
Parallel.For(0, 1000, i =>
66+
{
67+
try
68+
{
69+
if (i % 2 == 0)
70+
{
71+
collection.Add(i);
72+
}
73+
else
74+
{
75+
// RemoveAt can throw ArgumentOutOfRangeException if another
76+
// thread removed the last item between our Count check and
77+
// the actual removal. That's expected — just swallow it.
78+
try
79+
{
80+
if (collection.Count > 0)
81+
collection.RemoveAt(0);
82+
}
83+
catch (ArgumentOutOfRangeException) { }
84+
}
85+
}
86+
catch (Exception ex)
87+
{
88+
lock (exceptions) exceptions.Add(ex);
89+
}
90+
});
91+
92+
Assert.Empty(exceptions);
93+
}
94+
95+
[Fact]
96+
public async Task ConcurrentEnumeration_DuringInserts_ShouldNotThrow()
97+
{
98+
var collection = new ObservableCollection<int>();
99+
for (int i = 0; i < 100; i++) collection.Add(i);
100+
101+
var exceptions = new List<Exception>();
102+
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(3));
103+
104+
var writers = Task.Run(() =>
105+
{
106+
while (!cts.IsCancellationRequested)
107+
{
108+
try { collection.Add(Random.Shared.Next()); }
109+
catch (Exception ex) { lock (exceptions) exceptions.Add(ex); }
110+
}
111+
});
112+
113+
var readers = Task.Run(() =>
114+
{
115+
while (!cts.IsCancellationRequested)
116+
{
117+
try
118+
{
119+
foreach (var item in collection)
120+
{
121+
// Just read
122+
_ = item;
123+
}
124+
}
125+
catch (Exception ex) { lock (exceptions) exceptions.Add(ex); }
126+
}
127+
});
128+
129+
await Task.WhenAll(writers, readers);
130+
Assert.Empty(exceptions);
131+
}
132+
133+
[Fact]
134+
public async Task ConcurrentBinarySearch_DuringInserts_ShouldNotThrow()
135+
{
136+
var collection = new ObservableCollection<int>();
137+
for (int i = 0; i < 100; i++) collection.InsertSorted(i * 2); // even numbers
138+
139+
var exceptions = new List<Exception>();
140+
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(3));
141+
142+
var writers = Task.Run(() =>
143+
{
144+
int val = 201;
145+
while (!cts.IsCancellationRequested)
146+
{
147+
try { collection.InsertSorted(val += 2); }
148+
catch (Exception ex) { lock (exceptions) exceptions.Add(ex); }
149+
}
150+
});
151+
152+
var readers = Task.Run(() =>
153+
{
154+
while (!cts.IsCancellationRequested)
155+
{
156+
try { collection.BinarySearch(50); }
157+
catch (Exception ex) { lock (exceptions) exceptions.Add(ex); }
158+
}
159+
});
160+
161+
await Task.WhenAll(writers, readers);
162+
Assert.Empty(exceptions);
163+
}
164+
165+
[Fact]
166+
public void Enumerator_ReturnsAllItems()
167+
{
168+
var collection = new ObservableCollection<int> { 1, 2, 3, 4, 5 };
169+
var result = collection.ToList();
170+
Assert.Equal([1, 2, 3, 4, 5], result);
171+
}
172+
173+
[Fact]
174+
public void Enumerator_EmptyCollection_ReturnsNothing()
175+
{
176+
var collection = new ObservableCollection<int>();
177+
Assert.Empty(collection.ToList());
178+
}
179+
180+
[Fact]
181+
public void CollectionChanged_FiresOnInsert()
182+
{
183+
var collection = new ObservableCollection<int>();
184+
NotifyCollectionChangedEventArgs? args = null;
185+
collection.CollectionChanged += (_, e) => args = e;
186+
187+
collection.Add(42);
188+
189+
Assert.NotNull(args);
190+
Assert.Equal(NotifyCollectionChangedAction.Add, args.Action);
191+
Assert.Equal(42, args.NewItems![0]);
192+
}
193+
194+
[Fact]
195+
public void CollectionChanged_FiresOnRemove()
196+
{
197+
var collection = new ObservableCollection<int> { 10, 20, 30 };
198+
NotifyCollectionChangedEventArgs? args = null;
199+
collection.CollectionChanged += (_, e) => args = e;
200+
201+
collection.RemoveAt(1);
202+
203+
Assert.NotNull(args);
204+
Assert.Equal(NotifyCollectionChangedAction.Remove, args.Action);
205+
Assert.Equal(20, args.OldItems![0]);
206+
}
207+
208+
[Fact]
209+
public void CollectionChanged_FiresOnClear()
210+
{
211+
var collection = new ObservableCollection<int> { 1, 2, 3 };
212+
NotifyCollectionChangedEventArgs? args = null;
213+
collection.CollectionChanged += (_, e) => args = e;
214+
215+
collection.Clear();
216+
217+
Assert.NotNull(args);
218+
Assert.Equal(NotifyCollectionChangedAction.Reset, args.Action);
219+
Assert.Empty(collection);
220+
}
221+
222+
[Fact]
223+
public void Move_FiresCollectionChanged()
224+
{
225+
var collection = new ObservableCollection<int> { 1, 2, 3 };
226+
NotifyCollectionChangedEventArgs? args = null;
227+
collection.CollectionChanged += (_, e) => args = e;
228+
229+
collection.Move(0, 2);
230+
231+
Assert.NotNull(args);
232+
Assert.Equal(NotifyCollectionChangedAction.Move, args.Action);
233+
Assert.Equal(1, args.NewItems![0]);
234+
Assert.Equal([2, 3, 1], collection.ToList());
235+
}
236+
237+
[Fact]
238+
public void SetItem_FiresReplace()
239+
{
240+
var collection = new ObservableCollection<int> { 10, 20, 30 };
241+
NotifyCollectionChangedEventArgs? args = null;
242+
collection.CollectionChanged += (_, e) => args = e;
243+
244+
collection[1] = 99;
245+
246+
Assert.NotNull(args);
247+
Assert.Equal(NotifyCollectionChangedAction.Replace, args.Action);
248+
Assert.Equal(99, args.NewItems![0]);
249+
Assert.Equal(20, args.OldItems![0]);
250+
}
251+
252+
[Fact]
253+
public void SubscribersAvailable_TracksSubscriberCount()
254+
{
255+
var collection = new ObservableCollection<int>();
256+
var transitions = new List<bool>();
257+
collection.SubscribersAvailable += v => transitions.Add(v);
258+
259+
NotifyCollectionChangedEventHandler handler1 = (_, _) => { };
260+
NotifyCollectionChangedEventHandler handler2 = (_, _) => { };
261+
262+
collection.CollectionChanged += handler1; // 0→1: true
263+
collection.CollectionChanged += handler2; // 1→2: no event
264+
collection.CollectionChanged -= handler2; // 2→1: no event
265+
collection.CollectionChanged -= handler1; // 1→0: false
266+
267+
Assert.Equal([true, false], transitions);
268+
}
269+
270+
[Fact]
271+
public void ConcurrentGetOrAdd_SimulatesPipelinesModelPattern()
272+
{
273+
// This reproduces the exact crash pattern from PipelinesModel.GetOrCreate:
274+
// ConcurrentDictionary.GetOrAdd calling factory that inserts into ObservableCollection
275+
var dict = new System.Collections.Concurrent.ConcurrentDictionary<int, string>();
276+
var collection = new ObservableCollection<string>();
277+
var exceptions = new List<Exception>();
278+
279+
Parallel.For(0, 1000, i =>
280+
{
281+
try
282+
{
283+
dict.GetOrAdd(i % 100, key =>
284+
{
285+
var value = $"item-{key}";
286+
collection.Add(value);
287+
return value;
288+
});
289+
}
290+
catch (Exception ex)
291+
{
292+
lock (exceptions) exceptions.Add(ex);
293+
}
294+
});
295+
296+
Assert.Empty(exceptions);
297+
}
298+
}

Source/ModelingEvolution.Observable.sln

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,54 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ModelingEvolution.Observabl
77
EndProject
88
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ModelingEvolution.Observable.Blazor", "ModelingEvolution.Observable.Blazor\ModelingEvolution.Observable.Blazor.csproj", "{908733B1-8091-4F6E-9186-21D2EC3318BC}"
99
EndProject
10+
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "ModelingEvolution.Observable.Tests", "ModelingEvolution.Observable.Tests\ModelingEvolution.Observable.Tests.csproj", "{D5903E18-6429-482E-866F-1E679B1A5118}"
11+
EndProject
1012
Global
1113
GlobalSection(SolutionConfigurationPlatforms) = preSolution
1214
Debug|Any CPU = Debug|Any CPU
15+
Debug|x64 = Debug|x64
16+
Debug|x86 = Debug|x86
1317
Release|Any CPU = Release|Any CPU
18+
Release|x64 = Release|x64
19+
Release|x86 = Release|x86
1420
EndGlobalSection
1521
GlobalSection(ProjectConfigurationPlatforms) = postSolution
1622
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
1723
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Debug|Any CPU.Build.0 = Debug|Any CPU
24+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Debug|x64.ActiveCfg = Debug|Any CPU
25+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Debug|x64.Build.0 = Debug|Any CPU
26+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Debug|x86.ActiveCfg = Debug|Any CPU
27+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Debug|x86.Build.0 = Debug|Any CPU
1828
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Release|Any CPU.ActiveCfg = Release|Any CPU
1929
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Release|Any CPU.Build.0 = Release|Any CPU
30+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Release|x64.ActiveCfg = Release|Any CPU
31+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Release|x64.Build.0 = Release|Any CPU
32+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Release|x86.ActiveCfg = Release|Any CPU
33+
{6841B9E2-890D-4C48-BEDF-409F98813B19}.Release|x86.Build.0 = Release|Any CPU
2034
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
2135
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Debug|Any CPU.Build.0 = Debug|Any CPU
36+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Debug|x64.ActiveCfg = Debug|Any CPU
37+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Debug|x64.Build.0 = Debug|Any CPU
38+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Debug|x86.ActiveCfg = Debug|Any CPU
39+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Debug|x86.Build.0 = Debug|Any CPU
2240
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Release|Any CPU.ActiveCfg = Release|Any CPU
2341
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Release|Any CPU.Build.0 = Release|Any CPU
42+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Release|x64.ActiveCfg = Release|Any CPU
43+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Release|x64.Build.0 = Release|Any CPU
44+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Release|x86.ActiveCfg = Release|Any CPU
45+
{908733B1-8091-4F6E-9186-21D2EC3318BC}.Release|x86.Build.0 = Release|Any CPU
46+
{D5903E18-6429-482E-866F-1E679B1A5118}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
47+
{D5903E18-6429-482E-866F-1E679B1A5118}.Debug|Any CPU.Build.0 = Debug|Any CPU
48+
{D5903E18-6429-482E-866F-1E679B1A5118}.Debug|x64.ActiveCfg = Debug|Any CPU
49+
{D5903E18-6429-482E-866F-1E679B1A5118}.Debug|x64.Build.0 = Debug|Any CPU
50+
{D5903E18-6429-482E-866F-1E679B1A5118}.Debug|x86.ActiveCfg = Debug|Any CPU
51+
{D5903E18-6429-482E-866F-1E679B1A5118}.Debug|x86.Build.0 = Debug|Any CPU
52+
{D5903E18-6429-482E-866F-1E679B1A5118}.Release|Any CPU.ActiveCfg = Release|Any CPU
53+
{D5903E18-6429-482E-866F-1E679B1A5118}.Release|Any CPU.Build.0 = Release|Any CPU
54+
{D5903E18-6429-482E-866F-1E679B1A5118}.Release|x64.ActiveCfg = Release|Any CPU
55+
{D5903E18-6429-482E-866F-1E679B1A5118}.Release|x64.Build.0 = Release|Any CPU
56+
{D5903E18-6429-482E-866F-1E679B1A5118}.Release|x86.ActiveCfg = Release|Any CPU
57+
{D5903E18-6429-482E-866F-1E679B1A5118}.Release|x86.Build.0 = Release|Any CPU
2458
EndGlobalSection
2559
GlobalSection(SolutionProperties) = preSolution
2660
HideSolutionNode = FALSE

0 commit comments

Comments
 (0)