Skip to content

Commit

Permalink
Use ConcurrentDictionary to avoid threading issues (#104407)
Browse files Browse the repository at this point in the history
  • Loading branch information
@steveharter
steveharter authored Jul 8, 2024
1 parent 1c51cf0 commit 670d11f
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 58 deletions.
1 change: 1 addition & 0 deletions ...raries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@
<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj" ReferenceOutputAssembly="false" SetTargetFramework="TargetFramework=netstandard2.0" OutputItemType="Analyzer" />
<Reference Include="System.Collections" />
<Reference Include="System.Collections.Concurrent" />
<Reference Include="System.Collections.NonGeneric" />
<Reference Include="System.Collections.Specialized" />
<Reference Include="System.ComponentModel" />
Expand Down
35 changes: 22 additions & 13 deletions ...aries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Threading;

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

private TypeConverter? _converter;
private Dictionary<object, EventHandler?>? _valueChangedHandlers;
private ConcurrentDictionary<object, EventHandler?>? _valueChangedHandlers;
private object?[]? _editors;
private Type[]? _editorTypes;
private int _editorCount;
private object? _syncObject;

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

private object SyncObject => LazyInitializer.EnsureInitialized(ref _syncObject);

/// <summary>
/// When overridden in a derived class, gets the type of the
/// component this property is bound to.
Expand Down Expand Up @@ -165,10 +170,11 @@ public virtual void AddValueChanged(object component, EventHandler handler)
ArgumentNullException.ThrowIfNull(component);
ArgumentNullException.ThrowIfNull(handler);

_valueChangedHandlers ??= new Dictionary<object, EventHandler?>();

EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
_valueChangedHandlers[component] = (EventHandler?)Delegate.Combine(h, handler);
lock (SyncObject)
{
_valueChangedHandlers ??= new ConcurrentDictionary<object, EventHandler?>(concurrencyLevel: 1, capacity: 0);
_valueChangedHandlers.AddOrUpdate(component, handler, (k, v) => (EventHandler?)Delegate.Combine(v, handler));
}
}

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

if (_valueChangedHandlers != null)
{
EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
h = (EventHandler?)Delegate.Remove(h, handler);
if (h != null)
{
_valueChangedHandlers[component] = h;
}
else
lock (SyncObject)
{
_valueChangedHandlers.Remove(component);
EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null);
h = (EventHandler?)Delegate.Remove(h, handler);
if (h != null)
{
_valueChangedHandlers[component] = h;
}
else
{
_valueChangedHandlers.Remove(component, out EventHandler? _);
}
}
}
}
Expand Down
48 changes: 15 additions & 33 deletions ....ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections;
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.ComponentModel.Design;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
Expand All @@ -23,7 +24,7 @@ namespace System.ComponentModel
internal sealed partial class ReflectTypeDescriptionProvider : TypeDescriptionProvider
{
// ReflectedTypeData contains all of the type information we have gathered for a given type.
private Dictionary<Type, ReflectedTypeData>? _typeData;
private readonly ConcurrentDictionary<Type, ReflectedTypeData> _typeData = new ConcurrentDictionary<Type, ReflectedTypeData>();

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

lock (TypeDescriptor.s_commonSyncObject)
foreach (KeyValuePair<Type, ReflectedTypeData> kvp in _typeData)
{
Dictionary<Type, ReflectedTypeData>? typeData = _typeData;
if (typeData != null)
if (kvp.Key.Module == module && kvp.Value!.IsPopulated)
{
foreach (KeyValuePair<Type, ReflectedTypeData> kvp in typeData)
{
if (kvp.Key.Module == module && kvp.Value!.IsPopulated)
{
typeList.Add(kvp.Key);
}
}
typeList.Add(kvp.Key);
}
}

Expand Down Expand Up @@ -927,17 +920,15 @@ public override Type GetReflectionType(
/// </summary>
private ReflectedTypeData? GetTypeData([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type, bool createIfNeeded)
{
ReflectedTypeData? td = null;

if (_typeData != null && _typeData.TryGetValue(type, out td))
if (_typeData.TryGetValue(type, out ReflectedTypeData? td))
{
Debug.Assert(td != null);
return td;
}

lock (TypeDescriptor.s_commonSyncObject)
{
if (_typeData != null && _typeData.TryGetValue(type, out td))
if (_typeData.TryGetValue(type, out td))
{
Debug.Assert(td != null);

Expand All @@ -958,7 +949,6 @@ public override Type GetReflectionType(
if (createIfNeeded)
{
td = new ReflectedTypeData(type, isRegisteredType: false);
_typeData ??= new Dictionary<Type, ReflectedTypeData>();
_typeData[type] = td;
}
}
Expand All @@ -968,7 +958,7 @@ public override Type GetReflectionType(

private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)
{
if (_typeData == null || !_typeData.TryGetValue(type, out ReflectedTypeData? td))
if (!_typeData.TryGetValue(type, out ReflectedTypeData? td))
{
if (IsIntrinsicType(type))
{
Expand All @@ -991,49 +981,41 @@ private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)
public override void RegisterType<[DynamicallyAccessedMembers(TypeDescriptor.RegisteredTypesDynamicallyAccessedMembers)] T>()
{
Type componentType = typeof(T);
ReflectedTypeData? td = null;

if (_typeData != null && _typeData.ContainsKey(componentType))
if (_typeData.ContainsKey(componentType))
{
return;
}

lock (TypeDescriptor.s_commonSyncObject)
{
if (_typeData != null && _typeData.ContainsKey(componentType))
if (_typeData.ContainsKey(componentType))
{
return;
}

if (td == null)
{
td = new ReflectedTypeData(componentType, isRegisteredType: true);
_typeData ??= new Dictionary<Type, ReflectedTypeData>();
_typeData[componentType] = td;
}
ReflectedTypeData td = new ReflectedTypeData(componentType, isRegisteredType: true);
_typeData[componentType] = td;
}
}

private ReflectedTypeData GetOrRegisterType(Type type)
{
ReflectedTypeData? td = null;

if (_typeData != null && _typeData.TryGetValue(type, out td))
if (_typeData.TryGetValue(type, out ReflectedTypeData? td))
{
return td;
}

lock (TypeDescriptor.s_commonSyncObject)
{
if (_typeData != null && _typeData.TryGetValue(type, out td))
if (_typeData.TryGetValue(type, out td))
{
return td;
}

if (td == null)
{
td = new ReflectedTypeData(type, isRegisteredType: true);
_typeData ??= new Dictionary<Type, ReflectedTypeData>();
_typeData[type] = td;
}
}
Expand Down
5 changes: 3 additions & 2 deletions ...libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel.Design;
Expand Down Expand Up @@ -56,12 +57,12 @@ public sealed class TypeDescriptor
internal static readonly object s_commonSyncObject = new object();

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

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

private static readonly object s_initializedDefaultProvider = new object();

Expand Down
38 changes: 28 additions & 10 deletions src/libraries/System.ComponentModel.TypeConverter/tests/PropertyDescriptorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,29 +28,47 @@ public void RaiseAddedValueChangedHandler()
var component = new DescriptorTestComponent();
var properties = TypeDescriptor.GetProperties(component.GetType());
PropertyDescriptor propertyDescriptor = properties.Find(nameof(component.Property), false);
var handlerWasCalled = false;
EventHandler valueChangedHandler = (_, __) => handlerWasCalled = true;
int handlerCalledCount = 0;

propertyDescriptor.AddValueChanged(component, valueChangedHandler);
propertyDescriptor.SetValue(component, int.MaxValue);
EventHandler valueChangedHandler1 = (_, __) => handlerCalledCount++;
EventHandler valueChangedHandler2 = (_, __) => handlerCalledCount++;

propertyDescriptor.AddValueChanged(component, valueChangedHandler1);

// Add case.
propertyDescriptor.SetValue(component, int.MaxValue); // Add to delegate.
Assert.Equal(1, handlerCalledCount);

Assert.True(handlerWasCalled);

propertyDescriptor.AddValueChanged(component, valueChangedHandler2);
propertyDescriptor.SetValue(component, int.MaxValue); // Update delegate.
Assert.Equal(3, handlerCalledCount);
}

[Fact]
public void RemoveAddedValueChangedHandler()
{
var component = new DescriptorTestComponent();
var properties = TypeDescriptor.GetProperties(component.GetType());
var handlerWasCalled = false;
EventHandler valueChangedHandler = (_, __) => handlerWasCalled = true;
int handlerCalledCount = 0;

EventHandler valueChangedHandler1 = (_, __) => handlerCalledCount++;
EventHandler valueChangedHandler2 = (_, __) => handlerCalledCount++;

PropertyDescriptor propertyDescriptor = properties.Find(nameof(component.Property), false);

propertyDescriptor.AddValueChanged(component, valueChangedHandler);
propertyDescriptor.RemoveValueChanged(component, valueChangedHandler);
propertyDescriptor.AddValueChanged(component, valueChangedHandler1);
propertyDescriptor.AddValueChanged(component, valueChangedHandler2);
propertyDescriptor.SetValue(component, int.MaxValue);
Assert.Equal(2, handlerCalledCount);

propertyDescriptor.SetValue(component, int.MaxValue);
Assert.Equal(4, handlerCalledCount);

Assert.False(handlerWasCalled);
propertyDescriptor.RemoveValueChanged(component, valueChangedHandler1);
propertyDescriptor.RemoveValueChanged(component, valueChangedHandler2);
propertyDescriptor.SetValue(component, int.MaxValue);
Assert.Equal(4, handlerCalledCount);
}

[Fact]
Expand Down

0 comments on commit 670d11f

Please sign in to comment.