diff --git a/src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj b/src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj index f8b998c751ef22..fd01b2a1614a4f 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj @@ -253,6 +253,7 @@ SetTargetFramework="TargetFramework=netstandard2.0" OutputItemType="Analyzer" /> + diff --git a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs index 51fad8dd06d90c..90db74959d1572 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs @@ -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 { @@ -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? _valueChangedHandlers; + private ConcurrentDictionary? _valueChangedHandlers; private object?[]? _editors; private Type[]? _editorTypes; private int _editorCount; + private object? _syncObject; /// /// Initializes a new instance of the class with the specified name and @@ -49,6 +52,8 @@ protected PropertyDescriptor(MemberDescriptor descr, Attribute[]? attrs) : base( { } + private object SyncObject => LazyInitializer.EnsureInitialized(ref _syncObject); + /// /// When overridden in a derived class, gets the type of the /// component this property is bound to. @@ -124,10 +129,11 @@ public virtual void AddValueChanged(object component, EventHandler handler) ArgumentNullException.ThrowIfNull(component); ArgumentNullException.ThrowIfNull(handler); - _valueChangedHandlers ??= new Dictionary(); - - EventHandler? h = _valueChangedHandlers.GetValueOrDefault(component, defaultValue: null); - _valueChangedHandlers[component] = (EventHandler?)Delegate.Combine(h, handler); + lock (SyncObject) + { + _valueChangedHandlers ??= new ConcurrentDictionary(concurrencyLevel: 1, capacity: 0); + _valueChangedHandlers.AddOrUpdate(component, handler, (k, v) => (EventHandler?)Delegate.Combine(v, handler)); + } } /// @@ -392,15 +398,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? _); + } } } } diff --git a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs index aa29c8173e72bc..7c9870b579d8d7 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs @@ -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; @@ -22,10 +23,8 @@ namespace System.ComponentModel /// internal sealed partial class ReflectTypeDescriptionProvider : TypeDescriptionProvider { - // Hastable of Type -> ReflectedTypeData. ReflectedTypeData contains all - // of the type information we have gathered for a given type. - // - private Hashtable? _typeData; + // ReflectedTypeData contains all of the type information we have gathered for a given type. + private readonly ConcurrentDictionary _typeData = new ConcurrentDictionary(); // 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; @@ -91,8 +90,6 @@ private static Type[] InitializeSkipInterfaceAttributeList() internal static Guid ExtenderProviderKey { get; } = Guid.NewGuid(); - - private static readonly object s_internalSyncObject = new object(); /// /// Creates a new ReflectTypeDescriptionProvider. The type is the /// type we will obtain type information for. @@ -243,7 +240,7 @@ internal static void AddEditorTable(Type editorBaseType, Hashtable table) Debug.Assert(table != null, "COMPAT: Editor table should not be null"); // don't throw; RTM didn't so we can't do it either. - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { Hashtable editorTables = EditorTables; if (!editorTables.ContainsKey(editorBaseType)) @@ -298,7 +295,6 @@ internal static void AddEditorTable(Type editorBaseType, Hashtable table) return obj ?? Activator.CreateInstance(objectType, args); } - /// /// Helper method to create editors and type converters. This checks to see if the /// type implements a Type constructor, and if it does it invokes that ctor. @@ -429,7 +425,7 @@ internal TypeConverter GetConverter([DynamicallyAccessedMembers(DynamicallyAcces // if (table == null) { - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { table = editorTables[editorBaseType]; if (table == null) @@ -837,22 +833,11 @@ internal Type[] GetPopulatedTypes(Module module) { List typeList = new List(); - lock (s_internalSyncObject) + foreach (KeyValuePair kvp in _typeData) { - Hashtable? typeData = _typeData; - if (typeData != null) + if (kvp.Key.Module == module && kvp.Value!.IsPopulated) { - // Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations. - IDictionaryEnumerator e = typeData.GetEnumerator(); - while (e.MoveNext()) - { - DictionaryEntry de = e.Entry; - Type type = (Type)de.Key; - if (type.Module == module && ((ReflectedTypeData)de.Value!).IsPopulated) - { - typeList.Add(type); - } - } + typeList.Add(kvp.Key); } } @@ -897,28 +882,23 @@ public override Type GetReflectionType( /// private ReflectedTypeData? GetTypeData([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type, bool createIfNeeded) { - ReflectedTypeData? td = null; - - if (_typeData != null) + if (_typeData.TryGetValue(type, out ReflectedTypeData? td)) { - td = (ReflectedTypeData?)_typeData[type]; - if (td != null) - { - return td; - } + Debug.Assert(td != null); + return td; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { - if (_typeData != null) + if (_typeData.TryGetValue(type, out td)) { - td = (ReflectedTypeData?)_typeData[type]; + Debug.Assert(td != null); + return td; } - if (td == null && createIfNeeded) + if (createIfNeeded) { td = new ReflectedTypeData(type); - _typeData ??= new Hashtable(); _typeData[type] = td; } } @@ -1006,7 +986,7 @@ internal static Attribute[] ReflectGetAttributes(Type type) return attrs; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { attrs = (Attribute[]?)attributeCache[type]; if (attrs == null) @@ -1034,7 +1014,7 @@ internal static Attribute[] ReflectGetAttributes(MemberInfo member) return attrs; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { attrs = (Attribute[]?)attributeCache[member]; if (attrs == null) @@ -1063,7 +1043,7 @@ private static EventDescriptor[] ReflectGetEvents( return events; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { events = (EventDescriptor[]?)eventCache[type]; if (events == null) @@ -1160,7 +1140,7 @@ private static PropertyDescriptor[] ReflectGetExtendedProperties(IExtenderProvid ReflectPropertyDescriptor[]? extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType]; if (extendedProperties == null) { - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType]; @@ -1240,7 +1220,7 @@ private static PropertyDescriptor[] ReflectGetProperties( return properties; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { properties = (PropertyDescriptor[]?)propertyCache[type]; diff --git a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs index 948e3f4b386f35..c9ef20510aa1e3 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs @@ -2,6 +2,8 @@ // 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; using System.Diagnostics; @@ -26,12 +28,23 @@ public sealed class TypeDescriptor // lock on it for thread safety. It is used from nearly // every call to this class, so it will be created soon after // class load anyway. - private static readonly WeakHashtable s_providerTable = new WeakHashtable(); // mapping of type or object hash to a provider list - private static readonly Hashtable s_providerTypeTable = new Hashtable(); // A direct mapping from type to provider. + private static readonly WeakHashtable s_providerTable = new WeakHashtable(); + + // This lock object protects access to several thread-unsafe areas below, and is a single lock object to prevent deadlocks. + // - During s_providerTypeTable access. + // - To act as a mutex for CheckDefaultProvider() when it needs to create the default provider, which may re-enter the above case. + // - For cache access in the ReflectTypeDescriptionProvider class which may re-enter the above case. + // - For logic added by consumers, such as custom provider, constructor and property logic, which may re-enter the above cases in unexpected ways. + internal static readonly object s_commonSyncObject = new object(); + + // A direct mapping from type to provider. + private static readonly ConcurrentDictionary s_providerTypeTable = new ConcurrentDictionary(); + + // Tracks DefaultTypeDescriptionProviderAttributes. + // A value of `null` indicates initialization is in progress. + // A value of s_initializedDefaultProvider indicates the provider is initialized. + private static readonly ConcurrentDictionary s_defaultProviderInitialized = new ConcurrentDictionary(); - private static readonly Hashtable s_defaultProviderInitialized = new Hashtable(); // A table of type -> object to track DefaultTypeDescriptionProviderAttributes. - // A value of `null` indicates initialization is in progress. - // A value of s_initializedDefaultProvider indicates the provider is initialized. private static readonly object s_initializedDefaultProvider = new object(); private static WeakHashtable? s_associationTable; @@ -179,7 +192,7 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type) ArgumentNullException.ThrowIfNull(provider); ArgumentNullException.ThrowIfNull(type); - lock (s_providerTable) + lock (s_commonSyncObject) { // Get the root node, hook it up, and stuff it back into // the provider cache. @@ -209,7 +222,7 @@ public static void AddProvider(TypeDescriptionProvider provider, object instance // Get the root node, hook it up, and stuff it back into // the provider cache. - lock (s_providerTable) + lock (s_commonSyncObject) { refreshNeeded = s_providerTable.ContainsKey(instance); TypeDescriptionNode node = NodeFor(instance, true); @@ -265,15 +278,12 @@ public static void AddProviderTransparent(TypeDescriptionProvider provider, obje /// private static void CheckDefaultProvider(Type type) { - if (s_defaultProviderInitialized[type] == s_initializedDefaultProvider) + if (s_defaultProviderInitialized.TryGetValue(type, out object? provider) && provider == s_initializedDefaultProvider) { return; } - // Lock on s_providerTable even though s_providerTable is not modified here. - // Using a single lock prevents deadlocks since other methods that call into or are called - // by this method also lock on s_providerTable and the ordering of the locks may be different. - lock (s_providerTable) + lock (s_commonSyncObject) { AddDefaultProvider(type); } @@ -281,7 +291,7 @@ private static void CheckDefaultProvider(Type type) /// /// Add the default provider, if it exists. - /// For threading, this is always called under a 'lock (s_providerTable)'. + /// For threading, this is always called under a 'lock (s_commonSyncObject)'. /// private static void AddDefaultProvider(Type type) { @@ -295,7 +305,7 @@ private static void AddDefaultProvider(Type type) // Immediately set this to null to indicate we are in progress setting the default provider for a type. // This prevents re-entrance to this method. - s_defaultProviderInitialized[type] = null; + s_defaultProviderInitialized.TryAdd(type, null); // Always use core reflection when checking for the default provider attribute. // If there is a provider, we probably don't want to build up our own cache state against the type. @@ -1475,8 +1485,10 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator) while (node == null) { - node = (TypeDescriptionNode?)s_providerTypeTable[searchType] ?? - (TypeDescriptionNode?)s_providerTable[searchType]; + if (!s_providerTypeTable.TryGetValue(searchType, out node)) + { + node = (TypeDescriptionNode?)s_providerTable[searchType]; + } if (node == null) { @@ -1484,7 +1496,7 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator) if (searchType == typeof(object) || baseType == null) { - lock (s_providerTable) + lock (s_commonSyncObject) { node = (TypeDescriptionNode?)s_providerTable[searchType]; @@ -1500,9 +1512,9 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator) else if (createDelegator) { node = new TypeDescriptionNode(new DelegatingTypeDescriptionProvider(baseType)); - lock (s_providerTable) + lock (s_commonSyncObject) { - s_providerTypeTable[searchType] = node; + s_providerTypeTable.TryAdd(searchType, node); } } else @@ -1603,7 +1615,7 @@ private static TypeDescriptionNode NodeFor(object instance, bool createDelegator /// private static void NodeRemove(object key, TypeDescriptionProvider provider) { - lock (s_providerTable) + lock (s_commonSyncObject) { TypeDescriptionNode? head = (TypeDescriptionNode?)s_providerTable[key]; TypeDescriptionNode? target = head; @@ -2124,7 +2136,7 @@ private static void Refresh(object component, bool refreshReflectionProvider) { Type type = component.GetType(); - lock (s_providerTable) + lock (s_commonSyncObject) { // ReflectTypeDescritionProvider is only bound to object, but we // need go to through the entire table to try to find custom @@ -2208,7 +2220,7 @@ public static void Refresh(Type type) bool found = false; - lock (s_providerTable) + lock (s_commonSyncObject) { // ReflectTypeDescritionProvider is only bound to object, but we // need go to through the entire table to try to find custom @@ -2273,7 +2285,7 @@ public static void Refresh(Module module) // each of these levels. Hashtable? refreshedTypes = null; - lock (s_providerTable) + lock (s_commonSyncObject) { // Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations. IDictionaryEnumerator e = s_providerTable.GetEnumerator(); diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/PropertyDescriptorTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/PropertyDescriptorTests.cs index f791d46824f829..4786d3966a7e18 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/tests/PropertyDescriptorTests.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/tests/PropertyDescriptorTests.cs @@ -28,13 +28,21 @@ 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] @@ -42,15 +50,25 @@ 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]