TypeDescriptor threading fixes (#101305)

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

@@ -28,7 +28,12 @@ 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 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 +80,6 @@ public sealed class TypeDescriptor
             Guid.NewGuid()  // events
         };
 
-        private static readonly object s_internalSyncObject = new object();
-
         private TypeDescriptor()
         {
         }
@@ -262,32 +265,43 @@ public static void AddProviderTransparent(TypeDescriptionProvider provider, obje
         /// </summary>
         private static void CheckDefaultProvider(Type type)
         {
-            if (s_defaultProviders.ContainsKey(type))
+            if (s_defaultProviderInitialized[type] == s_initializedDefaultProvider)
             {
                 return;
             }
 
-            lock (s_internalSyncObject)
+            // 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)
             {
-                if (s_defaultProviders.ContainsKey(type))
-                {
-                    return;
-                }
+                AddDefaultProvider(type);
+            }
+        }
 
-                // 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;
+        /// <summary>
+        /// Add the default provider, if it exists.
+        /// For threading, this is always called under a 'lock (s_providerTable)'.
+        /// </summary>
+        private static void AddDefaultProvider(Type type)
+        {
+            bool providerAdded = false;
+
+            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,17 +320,19 @@ private static void CheckDefaultProvider(Type type)
                 Type? baseType = type.BaseType;
                 if (baseType != null && baseType != type)
                 {
-                    CheckDefaultProvider(baseType);
+                    AddDefaultProvider(baseType);
                 }
             }
+
+            s_defaultProviderInitialized[type] = s_initializedDefaultProvider;
         }
 
         /// <summary>
-        /// The CreateAssocation method creates an association between two objects.
+        /// The CreateAssociation method creates an association between two objects.
         /// Once an association is created, a designer or other filtering mechanism
         /// can add properties that route to either object into the primary object's
         /// property set. When a property invocation is made against the primary
-        /// object, GetAssocation will be called to resolve the actual object
+        /// object, GetAssociation will be called to resolve the actual object
         /// instance that is related to its type parameter.
         /// </summary>
         [EditorBrowsable(EditorBrowsableState.Advanced)]

src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs

@@ -5,6 +5,10 @@
 using System.Collections.Generic;
 using System.ComponentModel.Design;
 using System.Globalization;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.DotNet.RemoteExecutor;
 using Moq;
 using Xunit;
 
@@ -1231,5 +1235,248 @@ 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.");
+            }
+        }
+
+        [SkipOnPlatform(TestPlatforms.Browser, "Thread.Start is not supported on browsers.")]
+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public static void ConcurrentAddProviderAndGetProvider()
+        {
+            // Use a timeout value lower than RemoteExecutor in order to get a nice Fail message.
+            const int Timeout = 50000;
+
+            RemoteInvokeOptions options = new()
+            {
+                TimeOut = 60000
+            };
+
+            RemoteExecutor.Invoke(() =>
+            {
+                using var finished = new CountdownEvent(2);
+                Thread t1 = new Thread(() =>
+                {
+                    ConcurrentAddProvider();
+                    finished.Signal();
+                });
+                Thread t2 = new Thread(() =>
+                {
+                    ConcurrentGetProvider();
+                    finished.Signal();
+                });
+                t1.Start();
+                t2.Start();
+                finished.Wait(Timeout);
+                if (finished.CurrentCount != 0)
+                {
+                    Assert.Fail("Timeout. Possible deadlock.");
+                }
+            }, options).Dispose();
+
+            static void ConcurrentAddProvider()
+            {
+                var provider = new EmptyPropertiesTypeProvider();
+                TypeDescriptor.AddProvider(provider, typeof(MyClass));
+
+                // This test primarily verifies no deadlock, but verify the values anyway.
+                Assert.True(TypeDescriptor.GetProvider(typeof(MyClass)).IsSupportedType(typeof(MyClass)));
+            }
+
+            static void ConcurrentGetProvider()
+            {
+                TypeDescriptionProvider provider = TypeDescriptor.GetProvider(typeof(TypeWithProperty));
+
+                // This test primarily verifies no deadlock, but verify the values anyway.
+                Assert.True(provider.IsSupportedType(typeof(TypeWithProperty)));
+            }
+        }
+
+        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<object[]> 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<object[]> 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<TypeDescriptionProvider>(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
+        {
+        }
     }
 }