Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ConcurrentDictionary to avoid threading issues #104407

Merged
merged 7 commits into from
Jul 8, 2024
Merged

Use ConcurrentDictionary to avoid threading issues #104407

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9027610
Use ConcurrentDictionary to avoid threading issues
steveharter Jul 3, 2024
fb391a7
Use LazyInitializer.EnsureInitialized()
steveharter Jul 5, 2024
53e140d
Add test and fix issue with AddValueChanged
steveharter Jul 5, 2024
8fc2719
Use Interlocked.CompareExchange instead of a lock for Update
steveharter Jul 5, 2024
570ad78
Revert "Use Interlocked.CompareExchange instead of a lock for Update"
steveharter Jul 5, 2024
791aac2
Set concurrencyLevel=1 on dictionary
steveharter Jul 8, 2024
ae213ae
Remove unnecessary lock due to use of ConcurrentDictionary; change ca…
steveharter Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...raries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj
View file
Open in desktop
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
Open in desktop
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?>();
steveharter marked this conversation as resolved.
Show resolved Hide resolved
_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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lock (SyncObject)
while (true)
{
EventHandler? existingHandler;
if (!_valueChangedHandlers.TryGetValue(component, out existingHandler))
{
break;
}
EventHandler? replacementHandler = (EventHandler?)Delegate.Remove(existingHandler, handler);
if (replacementHandler is null)
{
if (_valueChangedHandlers.TryRemove(new KeyValuePair<object, EventHandler?>(component, existingHandler)))
{
break;
}
}
else
{
if (_valueChangedHandlers.TryUpdate(component, replacementHandler, existingHandler))
{
break;
}
}
}

I intend for this to replace the entire lock block, but GitHub won't let me make that suggestion. See here for what it should look like: https://gist.github.com/AustinWise/92f293463124cdbf95e702946502b636#file-propertydescriptor-cs-L62

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using the lock as-is is more straightforward. Also, the Remove is not likely to be used as often.

{
_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
45 changes: 15 additions & 30 deletions ....ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs
View file
Open in desktop
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 @@ -864,15 +864,11 @@ internal Type[] GetPopulatedTypes(Module module)

lock (TypeDescriptor.s_commonSyncObject)
steveharter marked this conversation as resolved.
Show resolved Hide resolved
{
Dictionary<Type, ReflectedTypeData>? typeData = _typeData;
if (typeData != null)
foreach (KeyValuePair<Type, ReflectedTypeData> kvp in _typeData)
{
foreach (KeyValuePair<Type, ReflectedTypeData> kvp in typeData)
if (kvp.Key.Module == module && kvp.Value!.IsPopulated)
{
if (kvp.Key.Module == module && kvp.Value!.IsPopulated)
{
typeList.Add(kvp.Key);
}
typeList.Add(kvp.Key);
}
}
}
Expand Down Expand Up @@ -927,17 +923,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 +952,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 +961,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 +984,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
Open in desktop
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
Open in desktop
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