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 2efd53e6223ae7..69da04776057ed 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs @@ -29,6 +29,7 @@ public sealed class TypeDescriptor 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 Hashtable s_defaultProvidersChecked = new Hashtable(); // A table similar to s_defaultProviders but only set after providers are checked, in order to reduce locks. 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. @@ -262,7 +263,9 @@ public static void AddProviderTransparent(TypeDescriptionProvider provider, obje /// private static void CheckDefaultProvider(Type type) { - if (s_defaultProviders.ContainsKey(type)) + bool providerAdded = false; + + if (s_defaultProvidersChecked.ContainsKey(type)) { return; } @@ -278,26 +281,27 @@ private static void CheckDefaultProvider(Type type) // and it starts messing around with type information, // this could infinitely recurse. s_defaultProviders[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. 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; + // 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 precedence. + object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), 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; + } } + + s_defaultProvidersChecked[type] = null; } // If we did not add a provider, check the base class. diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs index 299d73cfe31502..fcde40ee1547ac 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs @@ -5,6 +5,9 @@ using System.Collections.Generic; using System.ComponentModel.Design; using System.Globalization; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; using Moq; using Xunit; @@ -1231,5 +1234,196 @@ protected DerivedCultureInfo() : base("hello") class TwiceDerivedCultureInfo : DerivedCultureInfo { } + + private long _concurrentError = 0; + private bool ConcurrentError + { + get => Interlocked.Read(ref _concurrentError) == 1; + set => Interlocked.Exchange(ref _concurrentError, value ? 1 : 0); + } + + private void ConcurrentTest(TypeWithProperty instance) + { + var properties = TypeDescriptor.GetProperties(instance); + Thread.Sleep(10); + if (properties.Count > 0) + { + ConcurrentError = true; + } + } + + [SkipOnPlatform(TestPlatforms.Browser, "Thread.Start is not supported on browsers.")] + [Fact] + public void ConcurrentGetProperties_ReturnsExpected() + { + const int Timeout = 60000; + int concurrentCount = Environment.ProcessorCount * 2; + + using var finished = new CountdownEvent(concurrentCount); + + var instances = new TypeWithProperty[concurrentCount]; + for (int i = 0; i < concurrentCount; i++) + { + instances[i] = new TypeWithProperty(); + } + + for (int i = 0; i < concurrentCount; i++) + { + int i2 = i; + new Thread(() => + { + ConcurrentTest(instances[i2]); + finished.Signal(); + }).Start(); + } + + finished.Wait(Timeout); + + if (finished.CurrentCount != 0) + { + Assert.Fail("Timeout. Possible deadlock."); + } + else + { + Assert.False(ConcurrentError, "Fallback type descriptor is used. Possible race condition."); + } + } + + public sealed class EmptyPropertiesTypeProvider : TypeDescriptionProvider + { + private sealed class EmptyPropertyListDescriptor : ICustomTypeDescriptor + { + public AttributeCollection GetAttributes() => AttributeCollection.Empty; + + public string? GetClassName() => null; + + public string? GetComponentName() => null; + + public TypeConverter? GetConverter() => new TypeConverter(); + + public EventDescriptor? GetDefaultEvent() => null; + + public PropertyDescriptor? GetDefaultProperty() => null; + + public object? GetEditor(Type editorBaseType) => null; + + public EventDescriptorCollection GetEvents() => EventDescriptorCollection.Empty; + + public EventDescriptorCollection GetEvents(Attribute[]? attributes) => GetEvents(); + + public PropertyDescriptorCollection GetProperties() => PropertyDescriptorCollection.Empty; + + public PropertyDescriptorCollection GetProperties(Attribute[]? attributes) => GetProperties(); + + public object? GetPropertyOwner(PropertyDescriptor? pd) => null; + } + public override ICustomTypeDescriptor GetTypeDescriptor(Type objectType, object? instance) + { + return new EmptyPropertyListDescriptor(); + } + } + + [TypeDescriptionProvider(typeof(EmptyPropertiesTypeProvider))] + public sealed class TypeWithProperty + { + public int OneProperty { get; set; } + } + + 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 + { + } } }