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 874fa6e2e7d03e..069bec0873439c 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,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections; +using System.Collections.Generic; using System.Collections.Specialized; using System.ComponentModel.Design; using System.Diagnostics; @@ -28,7 +29,7 @@ public sealed class TypeDescriptor // 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 Hashtable s_defaultProviders = new Hashtable(); // A table of type -> default provider to track DefaultTypeDescriptionProviderAttributes. + private static readonly Dictionary s_processedTypes = new Dictionary(); private static WeakHashtable? s_associationTable; private static int s_metadataVersion; // a version stamp for our metadata. Used by property descriptors to know when to rebuild attributes. @@ -75,7 +76,8 @@ public sealed class TypeDescriptor Guid.NewGuid() // events }; - private static readonly object s_internalSyncObject = new object(); + private static readonly object s_processedTypesLock = new object(); + private static readonly ReaderWriterLockSlim s_providerTableLock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion); private TypeDescriptor() { @@ -176,7 +178,8 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type) ArgumentNullException.ThrowIfNull(provider); ArgumentNullException.ThrowIfNull(type); - lock (s_providerTable) + s_providerTableLock.EnterWriteLock(); + try { // Get the root node, hook it up, and stuff it back into // the provider cache. @@ -185,6 +188,10 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type) s_providerTable[type] = head; s_providerTypeTable.Clear(); } + finally + { + s_providerTableLock.ExitWriteLock(); + } Refresh(type); } @@ -204,15 +211,26 @@ public static void AddProvider(TypeDescriptionProvider provider, object instance bool refreshNeeded; - // Get the root node, hook it up, and stuff it back into - // the provider cache. - lock (s_providerTable) + s_providerTableLock.EnterUpgradeableReadLock(); + try { refreshNeeded = s_providerTable.ContainsKey(instance); TypeDescriptionNode node = NodeFor(instance, true); var head = new TypeDescriptionNode(provider) { Next = node }; - s_providerTable.SetWeak(instance, head); - s_providerTypeTable.Clear(); + s_providerTableLock.EnterWriteLock(); + try + { + s_providerTable.SetWeak(instance, head); + s_providerTypeTable.Clear(); + } + finally + { + s_providerTableLock.ExitWriteLock(); + } + } + finally + { + s_providerTableLock.ExitUpgradeableReadLock(); } if (refreshNeeded) @@ -262,51 +280,49 @@ public static void AddProviderTransparent(TypeDescriptionProvider provider, obje /// private static void CheckDefaultProvider(Type type) { - if (s_defaultProviders.ContainsKey(type)) + if (!s_processedTypes.TryGetValue(type, out ResetEventContext? context)) { - return; - } - - lock (s_internalSyncObject) - { - if (s_defaultProviders.ContainsKey(type)) + lock (s_processedTypesLock) { - return; - } + if (!s_processedTypes.ContainsKey(type)) + { + ManualResetEvent resetEventForAdd = new ManualResetEvent(false); + ResetEventContext contextForAdd = new ResetEventContext(resetEventForAdd, Environment.CurrentManagedThreadId); + s_processedTypes.Add(type, contextForAdd); - // Immediately clear this. If we find a default provider - // and it starts messing around with type information, - // this could infinitely recurse. - s_defaultProviders[type] = null; - } + var providerAttr = type.GetCustomAttribute(false); + bool providerAdded = false; - // 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. There shouldn't be - // more than one of these, but walk anyway. Walk in - // reverse order so that the most derived takes precidence. - object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false); - bool providerAdded = false; - for (int idx = attrs.Length - 1; idx >= 0; idx--) - { - TypeDescriptionProviderAttribute pa = (TypeDescriptionProviderAttribute)attrs[idx]; - Type? providerType = Type.GetType(pa.TypeName); - if (providerType != null && typeof(TypeDescriptionProvider).IsAssignableFrom(providerType)) - { - TypeDescriptionProvider prov = (TypeDescriptionProvider)Activator.CreateInstance(providerType)!; - AddProvider(prov, type); - providerAdded = true; + if (providerAttr != null) + { + Type? providerType = Type.GetType(providerAttr.TypeName); + if (providerType != null && typeof(TypeDescriptionProvider).IsAssignableFrom(providerType)) + { + TypeDescriptionProvider provider = (TypeDescriptionProvider)Activator.CreateInstance(providerType)!; + AddProvider(provider, type); + providerAdded = true; + } + } + + // If we did not add a provider, check the base class. + if (!providerAdded) + { + Type? baseType = type.BaseType; + if (baseType != null && baseType != type) + { + CheckDefaultProvider(baseType); + } + } + + resetEventForAdd.Set(); + } } } - - // If we did not add a provider, check the base class. - if (!providerAdded) + else { - Type? baseType = type.BaseType; - if (baseType != null && baseType != type) + if (context.OwnerId != Environment.CurrentManagedThreadId) { - CheckDefaultProvider(baseType); + context.ResetEvent.WaitOne(); } } } @@ -1457,45 +1473,63 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator) TypeDescriptionNode? node = null; Type searchType = type; - while (node == null) + s_providerTableLock.EnterUpgradeableReadLock(); + try { - node = (TypeDescriptionNode?)s_providerTypeTable[searchType] ?? - (TypeDescriptionNode?)s_providerTable[searchType]; - - if (node == null) + while (node == null) { - Type? baseType = GetNodeForBaseType(searchType); + node = (TypeDescriptionNode?)s_providerTypeTable[searchType] ?? + (TypeDescriptionNode?)s_providerTable[searchType]; - if (searchType == typeof(object) || baseType == null) + if (node == null) { - lock (s_providerTable) + Type? baseType = GetNodeForBaseType(searchType); + + if (searchType == typeof(object) || baseType == null) { - node = (TypeDescriptionNode?)s_providerTable[searchType]; + s_providerTableLock.EnterWriteLock(); + try + { + node = (TypeDescriptionNode?)s_providerTable[searchType]; - if (node == null) + if (node == null) + { + // The reflect type description provider is a default provider that + // can provide type information for all objects. + node = new TypeDescriptionNode(new ReflectTypeDescriptionProvider()); + s_providerTable[searchType] = node; + } + } + finally { - // The reflect type description provider is a default provider that - // can provide type information for all objects. - node = new TypeDescriptionNode(new ReflectTypeDescriptionProvider()); - s_providerTable[searchType] = node; + s_providerTableLock.ExitWriteLock(); } } - } - else if (createDelegator) - { - node = new TypeDescriptionNode(new DelegatingTypeDescriptionProvider(baseType)); - lock (s_providerTable) + else if (createDelegator) { - s_providerTypeTable[searchType] = node; + node = new TypeDescriptionNode(new DelegatingTypeDescriptionProvider(baseType)); + s_providerTableLock.EnterWriteLock(); + try + { + s_providerTypeTable[searchType] = node; + } + finally + { + s_providerTableLock.ExitWriteLock(); + } + } + else + { + // Continue our search + searchType = baseType; } - } - else - { - // Continue our search - searchType = baseType; } } } + finally + { + s_providerTableLock.ExitUpgradeableReadLock(); + } return node; } @@ -1587,7 +1621,8 @@ private static TypeDescriptionNode NodeFor(object instance, bool createDelegator /// private static void NodeRemove(object key, TypeDescriptionProvider provider) { - lock (s_providerTable) + s_providerTableLock.EnterUpgradeableReadLock(); + try { TypeDescriptionNode? head = (TypeDescriptionNode?)s_providerTable[key]; TypeDescriptionNode? target = head; @@ -1623,7 +1658,15 @@ private static void NodeRemove(object key, TypeDescriptionProvider provider) if (target == head && target.Provider is DelegatingTypeDescriptionProvider) { Debug.Assert(target.Next == null, "Delegating provider should always be the last provider in the chain."); - s_providerTable.Remove(key); + s_providerTableLock.EnterWriteLock(); + try + { + s_providerTable.Remove(key); + } + finally + { + s_providerTableLock.ExitWriteLock(); + } } } else if (target != head) @@ -1645,7 +1688,15 @@ private static void NodeRemove(object key, TypeDescriptionProvider provider) } else { - s_providerTable.Remove(key); + s_providerTableLock.EnterWriteLock(); + try + { + s_providerTable.Remove(key); + } + finally + { + s_providerTableLock.ExitWriteLock(); + } } // Finally, clear our cache of provider types; it might be invalid @@ -1653,6 +1704,10 @@ private static void NodeRemove(object key, TypeDescriptionProvider provider) s_providerTypeTable.Clear(); } } + finally + { + s_providerTableLock.ExitUpgradeableReadLock(); + } } /// @@ -2107,8 +2162,9 @@ private static void Refresh(object component, bool refreshReflectionProvider) if (refreshReflectionProvider) { Type type = component.GetType(); + s_providerTableLock.EnterReadLock(); - lock (s_providerTable) + try { // ReflectTypeDescritionProvider is only bound to object, but we // need go to through the entire table to try to find custom @@ -2141,6 +2197,10 @@ private static void Refresh(object component, bool refreshReflectionProvider) } } } + finally + { + s_providerTableLock.ExitReadLock(); + } } // We need to clear our filter even if no typedescriptionprovider had data. @@ -2192,7 +2252,8 @@ public static void Refresh(Type type) bool found = false; - lock (s_providerTable) + s_providerTableLock.EnterReadLock(); + try { // ReflectTypeDescritionProvider is only bound to object, but we // need go to through the entire table to try to find custom @@ -2225,6 +2286,10 @@ public static void Refresh(Type type) } } } + finally + { + s_providerTableLock.ExitReadLock(); + } // We only clear our filter and fire the refresh event if there was one or // more type description providers that were populated with metdata. @@ -2257,7 +2322,8 @@ public static void Refresh(Module module) // each of these levels. Hashtable? refreshedTypes = null; - lock (s_providerTable) + s_providerTableLock.EnterReadLock(); + try { // Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations. IDictionaryEnumerator e = s_providerTable.GetEnumerator(); @@ -2290,6 +2356,10 @@ public static void Refresh(Module module) } } } + finally + { + s_providerTableLock.ExitReadLock(); + } // And raise the event if types were refresh and handlers are attached. if (refreshedTypes != null && Refreshed != null) @@ -2996,6 +3066,19 @@ PropertyDescriptorCollection ICustomTypeDescriptor.GetProperties(Attribute[]? at } } + private sealed class ResetEventContext + { + public ResetEventContext(ManualResetEvent resetEvent, int ownerId) + { + ResetEvent = resetEvent; + OwnerId = ownerId; + } + + public ManualResetEvent ResetEvent { get; } + + public int OwnerId { get; } + } + /// /// This is a linked list node that is comprised of a type /// description provider. Each node contains a Next pointer diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs index 2044bd44b47b9f..7b7cfd7fb05b1a 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs @@ -3,6 +3,8 @@ using System.Collections; using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; using Moq; using Moq.Protected; using Xunit; @@ -808,6 +810,103 @@ public void IsSupportedType_NullTypeWithParent_ThrowsArgumentNullException() AssertExtensions.Throws("type", () => provider.IsSupportedType(null)); } + public static IEnumerable GetConverter_ByMultithread_ReturnsExpected_TestData() + { + yield return new object[] { typeof(MyClass), typeof(MyTypeConverter) }; + yield return new object[] { typeof(MyInheritedClassWithCustomTypeDescriptionProvider), typeof(MyInheritedClassWithCustomTypeDescriptionProviderConverter) }; + yield return new object[] { typeof(MyInheritedClassWithInheritedTypeDescriptionProvider), typeof(MyTypeConverter) }; + } + + [Theory] + [MemberData(nameof(GetConverter_ByMultithread_ReturnsExpected_TestData))] + public async void GetConverter_ByMultithread_ReturnsExpected(Type typeForGetConverter, Type expectedConverterType) + { + TypeConverter[] actualConverters = await Task.WhenAll( + Enumerable.Range(0, 100).Select(_ => + Task.Run(() => TypeDescriptor.GetConverter(typeForGetConverter)))); + Assert.All(actualConverters, + currentConverter => Assert.IsType(expectedConverterType, currentConverter)); + } + + public static IEnumerable GetConverterWithAddProvider_ByMultithread_Success_TestData() + { + foreach (object[] currentTestCase in GetConverter_ByMultithread_ReturnsExpected_TestData()) + { + yield return currentTestCase; + } + } + + [Theory] + [MemberData(nameof(GetConverterWithAddProvider_ByMultithread_Success_TestData))] + public async void GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) + { + TypeConverter[] actualConverters = await Task.WhenAll( + Enumerable.Range(0, 200).Select(_ => + Task.Run(() => + { + var mockProvider = new Mock(MockBehavior.Strict); + var someInstance = new object(); + TypeDescriptor.AddProvider(mockProvider.Object, someInstance); + return TypeDescriptor.GetConverter(typeForGetConverter); + }))); + Assert.All(actualConverters, + currentConverter => Assert.IsType(expectedConverterType, currentConverter)); + } + + [TypeDescriptionProvider(typeof(MyClassTypeDescriptionProvider))] + public class MyClass + { + } + + [TypeDescriptionProvider(typeof(MyInheritedClassWithCustomTypeDescriptionProviderTypeDescriptionProvider))] + public class MyInheritedClassWithCustomTypeDescriptionProvider : MyClass + { + } + + public class MyInheritedClassWithInheritedTypeDescriptionProvider : MyClass + { + } + + public class MyClassTypeDescriptionProvider : TypeDescriptionProvider + { + public override ICustomTypeDescriptor GetTypeDescriptor(Type objectType, object instance) + { + return new MyClassTypeDescriptor(); + } + } + + public class MyInheritedClassWithCustomTypeDescriptionProviderTypeDescriptionProvider : TypeDescriptionProvider + { + public override ICustomTypeDescriptor GetTypeDescriptor(Type objectType, object instance) + { + return new MyInheritedClassWithCustomTypeDescriptionProviderTypeDescriptor(); + } + } + + public class MyClassTypeDescriptor : CustomTypeDescriptor + { + public override TypeConverter GetConverter() + { + return new MyTypeConverter(); + } + } + + public class MyInheritedClassWithCustomTypeDescriptionProviderTypeDescriptor : CustomTypeDescriptor + { + public override TypeConverter GetConverter() + { + return new MyInheritedClassWithCustomTypeDescriptionProviderConverter(); + } + } + + public class MyTypeConverter : TypeConverter + { + } + + public class MyInheritedClassWithCustomTypeDescriptionProviderConverter : TypeConverter + { + } + private class SubTypeDescriptionProvider : TypeDescriptionProvider { public SubTypeDescriptionProvider() : base()