Skip to content

Commit 670d11f

Browse files
stevehartersteveharter
authored
Use ConcurrentDictionary to avoid threading issues (#104407)
1 parent 1c51cf0 commit 670d11f

File tree

5 files changed

+69
-58
lines changed

5 files changed

+69
-58
lines changed

‎src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@
250250
<ItemGroup>
251251
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj" ReferenceOutputAssembly="false" SetTargetFramework="TargetFramework=netstandard2.0" OutputItemType="Analyzer" />
252252
<Reference Include="System.Collections" />
253+
<Reference Include="System.Collections.Concurrent" />
253254
<Reference Include="System.Collections.NonGeneric" />
254255
<Reference Include="System.Collections.Specialized" />
255256
<Reference Include="System.ComponentModel" />

‎src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Collections;
5+
using System.Collections.Concurrent;
56
using System.Collections.Generic;
67
using System.Diagnostics.CodeAnalysis;
78
using System.Reflection;
9+
using System.Threading;
810

911
namespace System.ComponentModel
1012
{
@@ -16,10 +18,11 @@ public abstract class PropertyDescriptor : MemberDescriptor
1618
internal const string PropertyDescriptorPropertyTypeMessage = "PropertyDescriptor's PropertyType cannot be statically discovered.";
1719

1820
private TypeConverter? _converter;
19-
private Dictionary<object, EventHandler?>? _valueChangedHandlers;
21+
private ConcurrentDictionary<object, EventHandler?>? _valueChangedHandlers;
2022
private object?[]? _editors;
2123
private Type[]? _editorTypes;
2224
private int _editorCount;
25+
private object? _syncObject;
2326

2427
/// <summary>
2528
/// Initializes a new instance of the <see cref='System.ComponentModel.PropertyDescriptor'/> class with the specified name and
@@ -49,6 +52,8 @@ protected PropertyDescriptor(MemberDescriptor descr, Attribute[]? attrs) : base(
4952
{
5053
}
5154

55+
private object SyncObject => LazyInitializer.EnsureInitialized(ref _syncObject);
56+
5257
/// <summary>
5358
/// When overridden in a derived class, gets the type of the
5459
/// component this property is bound to.
@@ -165,10 +170,11 @@ public virtual void AddValueChanged(object component, EventHandler handler)
165170
ArgumentNullException.ThrowIfNull(component);
166171
ArgumentNullException.ThrowIfNull(handler);
167172

168-
_valueChangedHandlers ??= new Dictionary<object, EventHandler?>();
169-
170-
EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
171-
_valueChangedHandlers[component] = (EventHandler?)Delegate.Combine(h, handler);
173+
lock (SyncObject)
174+
{
175+
_valueChangedHandlers ??= new ConcurrentDictionary<object, EventHandler?>(concurrencyLevel: 1, capacity: 0);
176+
_valueChangedHandlers.AddOrUpdate(component, handler, (k, v) => (EventHandler?)Delegate.Combine(v, handler));
177+
}
172178
}
173179

174180
/// <summary>
@@ -433,15 +439,18 @@ public virtual void RemoveValueChanged(object component, EventHandler handler)
433439

434440
if (_valueChangedHandlers != null)
435441
{
436-
EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
437-
h = (EventHandler?)Delegate.Remove(h, handler);
438-
if (h != null)
439-
{
440-
_valueChangedHandlers[component] = h;
441-
}
442-
else
442+
lock (SyncObject)
443443
{
444-
_valueChangedHandlers.Remove(component);
444+
EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
445+
h = (EventHandler?)Delegate.Remove(h, handler);
446+
if (h != null)
447+
{
448+
_valueChangedHandlers[component] = h;
449+
}
450+
else
451+
{
452+
_valueChangedHandlers.Remove(component, out EventHandler? _);
453+
}
445454
}
446455
}
447456
}

‎src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs‎

Lines changed: 15 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System.Collections;
55
using System.Collections.Generic;
6+
using System.Collections.Concurrent;
67
using System.ComponentModel.Design;
78
using System.Diagnostics;
89
using System.Diagnostics.CodeAnalysis;
@@ -23,7 +24,7 @@ namespace System.ComponentModel
2324
internal sealed partial class ReflectTypeDescriptionProvider : TypeDescriptionProvider
2425
{
2526
// ReflectedTypeData contains all of the type information we have gathered for a given type.
26-
private Dictionary<Type, ReflectedTypeData>? _typeData;
27+
private readonly ConcurrentDictionary<Type, ReflectedTypeData> _typeData = new ConcurrentDictionary<Type, ReflectedTypeData>();
2728

2829
// This is the signature we look for when creating types that are generic, but
2930
// want to know what type they are dealing with. Enums are a good example of this;
@@ -281,8 +282,7 @@ internal static void AddEditorTable(Type editorBaseType, Hashtable table)
281282
public override bool? RequireRegisteredTypes => true;
282283
public override bool IsRegisteredType(Type type)
283284
{
284-
if (_typeData != null &&
285-
_typeData.TryGetValue(type, out ReflectedTypeData? data) &&
285+
if (_typeData.TryGetValue(type, out ReflectedTypeData? data) &&
286286
data.IsRegistered)
287287
{
288288
return true;
@@ -862,18 +862,11 @@ internal Type[] GetPopulatedTypes(Module module)
862862
{
863863
List<Type> typeList = new List<Type>();
864864

865-
lock (TypeDescriptor.s_commonSyncObject)
865+
foreach (KeyValuePair<Type, ReflectedTypeData> kvp in _typeData)
866866
{
867-
Dictionary<Type, ReflectedTypeData>? typeData = _typeData;
868-
if (typeData != null)
867+
if (kvp.Key.Module == module && kvp.Value!.IsPopulated)
869868
{
870-
foreach (KeyValuePair<Type, ReflectedTypeData> kvp in typeData)
871-
{
872-
if (kvp.Key.Module == module && kvp.Value!.IsPopulated)
873-
{
874-
typeList.Add(kvp.Key);
875-
}
876-
}
869+
typeList.Add(kvp.Key);
877870
}
878871
}
879872

@@ -927,17 +920,15 @@ public override Type GetReflectionType(
927920
/// </summary>
928921
private ReflectedTypeData? GetTypeData([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type, bool createIfNeeded)
929922
{
930-
ReflectedTypeData? td = null;
931-
932-
if (_typeData != null && _typeData.TryGetValue(type, out td))
923+
if (_typeData.TryGetValue(type, out ReflectedTypeData? td))
933924
{
934925
Debug.Assert(td != null);
935926
return td;
936927
}
937928

938929
lock (TypeDescriptor.s_commonSyncObject)
939930
{
940-
if (_typeData != null && _typeData.TryGetValue(type, out td))
931+
if (_typeData.TryGetValue(type, out td))
941932
{
942933
Debug.Assert(td != null);
943934

@@ -958,7 +949,6 @@ public override Type GetReflectionType(
958949
if (createIfNeeded)
959950
{
960951
td = new ReflectedTypeData(type, isRegisteredType: false);
961-
_typeData ??= new Dictionary<Type, ReflectedTypeData>();
962952
_typeData[type] = td;
963953
}
964954
}
@@ -968,7 +958,7 @@ public override Type GetReflectionType(
968958

969959
private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)
970960
{
971-
if (_typeData == null || !_typeData.TryGetValue(type, out ReflectedTypeData? td))
961+
if (!_typeData.TryGetValue(type, out ReflectedTypeData? td))
972962
{
973963
if (IsIntrinsicType(type))
974964
{
@@ -991,49 +981,41 @@ private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)
991981
public override void RegisterType<[DynamicallyAccessedMembers(TypeDescriptor.RegisteredTypesDynamicallyAccessedMembers)] T>()
992982
{
993983
Type componentType = typeof(T);
994-
ReflectedTypeData? td = null;
995984

996-
if (_typeData != null && _typeData.ContainsKey(componentType))
985+
if (_typeData.ContainsKey(componentType))
997986
{
998987
return;
999988
}
1000989

1001990
lock (TypeDescriptor.s_commonSyncObject)
1002991
{
1003-
if (_typeData != null && _typeData.ContainsKey(componentType))
992+
if (_typeData.ContainsKey(componentType))
1004993
{
1005994
return;
1006995
}
1007996

1008-
if (td == null)
1009-
{
1010-
td = new ReflectedTypeData(componentType, isRegisteredType: true);
1011-
_typeData ??= new Dictionary<Type, ReflectedTypeData>();
1012-
_typeData[componentType] = td;
1013-
}
997+
ReflectedTypeData td = new ReflectedTypeData(componentType, isRegisteredType: true);
998+
_typeData[componentType] = td;
1014999
}
10151000
}
10161001

10171002
private ReflectedTypeData GetOrRegisterType(Type type)
10181003
{
1019-
ReflectedTypeData? td = null;
1020-
1021-
if (_typeData != null && _typeData.TryGetValue(type, out td))
1004+
if (_typeData.TryGetValue(type, out ReflectedTypeData? td))
10221005
{
10231006
return td;
10241007
}
10251008

10261009
lock (TypeDescriptor.s_commonSyncObject)
10271010
{
1028-
if (_typeData != null && _typeData.TryGetValue(type, out td))
1011+
if (_typeData.TryGetValue(type, out td))
10291012
{
10301013
return td;
10311014
}
10321015

10331016
if (td == null)
10341017
{
10351018
td = new ReflectedTypeData(type, isRegisteredType: true);
1036-
_typeData ??= new Dictionary<Type, ReflectedTypeData>();
10371019
_typeData[type] = td;
10381020
}
10391021
}

‎src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Collections;
5+
using System.Collections.Concurrent;
56
using System.Collections.Generic;
67
using System.Collections.Specialized;
78
using System.ComponentModel.Design;
@@ -56,12 +57,12 @@ public sealed class TypeDescriptor
5657
internal static readonly object s_commonSyncObject = new object();
5758

5859
// A direct mapping from type to provider.
59-
private static readonly Dictionary<Type, TypeDescriptionNode> s_providerTypeTable = new Dictionary<Type, TypeDescriptionNode>();
60+
private static readonly ConcurrentDictionary<Type, TypeDescriptionNode> s_providerTypeTable = new ConcurrentDictionary<Type, TypeDescriptionNode>();
6061

6162
// Tracks DefaultTypeDescriptionProviderAttributes.
6263
// A value of `null` indicates initialization is in progress.
6364
// A value of s_initializedDefaultProvider indicates the provider is initialized.
64-
private static readonly Dictionary<Type, object?> s_defaultProviderInitialized = new Dictionary<Type, object?>();
65+
private static readonly ConcurrentDictionary<Type, object?> s_defaultProviderInitialized = new ConcurrentDictionary<Type, object?>();
6566

6667
private static readonly object s_initializedDefaultProvider = new object();
6768

‎src/libraries/System.ComponentModel.TypeConverter/tests/PropertyDescriptorTests.cs‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,29 +28,47 @@ public void RaiseAddedValueChangedHandler()
2828
var component = new DescriptorTestComponent();
2929
var properties = TypeDescriptor.GetProperties(component.GetType());
3030
PropertyDescriptor propertyDescriptor = properties.Find(nameof(component.Property), false);
31-
var handlerWasCalled = false;
32-
EventHandler valueChangedHandler = (_, __) => handlerWasCalled = true;
31+
int handlerCalledCount = 0;
3332

34-
propertyDescriptor.AddValueChanged(component, valueChangedHandler);
35-
propertyDescriptor.SetValue(component, int.MaxValue);
33+
EventHandler valueChangedHandler1 = (_, __) => handlerCalledCount++;
34+
EventHandler valueChangedHandler2 = (_, __) => handlerCalledCount++;
35+
36+
propertyDescriptor.AddValueChanged(component, valueChangedHandler1);
37+
38+
// Add case.
39+
propertyDescriptor.SetValue(component, int.MaxValue); // Add to delegate.
40+
Assert.Equal(1, handlerCalledCount);
3641

37-
Assert.True(handlerWasCalled);
42+
43+
propertyDescriptor.AddValueChanged(component, valueChangedHandler2);
44+
propertyDescriptor.SetValue(component, int.MaxValue); // Update delegate.
45+
Assert.Equal(3, handlerCalledCount);
3846
}
3947

4048
[Fact]
4149
public void RemoveAddedValueChangedHandler()
4250
{
4351
var component = new DescriptorTestComponent();
4452
var properties = TypeDescriptor.GetProperties(component.GetType());
45-
var handlerWasCalled = false;
46-
EventHandler valueChangedHandler = (_, __) => handlerWasCalled = true;
53+
int handlerCalledCount = 0;
54+
55+
EventHandler valueChangedHandler1 = (_, __) => handlerCalledCount++;
56+
EventHandler valueChangedHandler2 = (_, __) => handlerCalledCount++;
57+
4758
PropertyDescriptor propertyDescriptor = properties.Find(nameof(component.Property), false);
4859

49-
propertyDescriptor.AddValueChanged(component, valueChangedHandler);
50-
propertyDescriptor.RemoveValueChanged(component, valueChangedHandler);
60+
propertyDescriptor.AddValueChanged(component, valueChangedHandler1);
61+
propertyDescriptor.AddValueChanged(component, valueChangedHandler2);
62+
propertyDescriptor.SetValue(component, int.MaxValue);
63+
Assert.Equal(2, handlerCalledCount);
64+
5165
propertyDescriptor.SetValue(component, int.MaxValue);
66+
Assert.Equal(4, handlerCalledCount);
5267

53-
Assert.False(handlerWasCalled);
68+
propertyDescriptor.RemoveValueChanged(component, valueChangedHandler1);
69+
propertyDescriptor.RemoveValueChanged(component, valueChangedHandler2);
70+
propertyDescriptor.SetValue(component, int.MaxValue);
71+
Assert.Equal(4, handlerCalledCount);
5472
}
5573

5674
[Fact]

0 commit comments

Comments
 (0)