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 2efd53e6223ae..6043b69b4f80f 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs @@ -28,7 +28,13 @@ 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 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 readonly object s_defaultProviderSyncObject = new object(); + 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,8 +81,6 @@ public sealed class TypeDescriptor Guid.NewGuid() // events }; - private static readonly object s_internalSyncObject = new object(); - private TypeDescriptor() { } @@ -262,32 +266,40 @@ public static void AddProviderTransparent(TypeDescriptionProvider provider, obje /// private static void CheckDefaultProvider(Type type) { - if (s_defaultProviders.ContainsKey(type)) + if (s_defaultProviderInitialized[type] == s_initializedDefaultProvider) { return; } - lock (s_internalSyncObject) + lock (s_defaultProviderSyncObject) { - if (s_defaultProviders.ContainsKey(type)) - { - return; - } + AddDefaultProvider(type); + } + } + + /// + /// Add the default provider, if it exists. + /// For threading, this is always called under a 'lock (s_defaultProviderSyncObject)'. + /// + private static void AddDefaultProvider(Type type) + { + bool providerAdded = false; - // 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; + if (s_defaultProviderInitialized.ContainsKey(type)) + { + // Either another thread finished initializing for this type, or we are recursing on the same thread. + return; } - // 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. + // 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; + + // 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); - bool providerAdded = false; for (int idx = attrs.Length - 1; idx >= 0; idx--) { TypeDescriptionProviderAttribute pa = (TypeDescriptionProviderAttribute)attrs[idx]; @@ -306,9 +318,11 @@ private static void CheckDefaultProvider(Type type) Type? baseType = type.BaseType; if (baseType != null && baseType != type) { - CheckDefaultProvider(baseType); + AddDefaultProvider(baseType); } } + + s_defaultProviderInitialized[type] = s_initializedDefaultProvider; } /// diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs index 299d73cfe3150..fcde40ee1547a 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 + { + } } }