Make TypeDescriptor thread safe with custom providers and minimize lock use

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

@@ -189,6 +189,30 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type)
             Refresh(type);
         }
 
+        /// <summary>
+        /// Specialized version for calls from <see cref="CheckDefaultProvider(Type)"/>.
+        /// Code is adjusted to minimize lock in the aforementioned method.
+        /// </summary>
+        /// <param name="provider"></param>
+        /// <param name="type"></param>
+        private static void AddProvider_FewerLock(TypeDescriptionProvider provider, Type type)
+        {
+            Debug.Assert(provider != null);
+            Debug.Assert(type != null);
+
+            lock (s_providerTable)
+            {
+                // Get the root node, hook it up, and stuff it back into
+                // the provider cache.
+                TypeDescriptionNode node = NodeFor_LockFreeWithCreateDelegator(type);
+                var head = new TypeDescriptionNode(provider) { Next = node };
+                s_providerTable[type] = head;
+                s_providerTypeTable.Clear();
+            }
+
+            Refresh(type);
+        }
+
         /// <summary>
         /// Adds a type description provider that will be called on to provide
         /// type information for a single object instance. A provider added
@@ -262,44 +286,37 @@ public static void AddProviderTransparent(TypeDescriptionProvider provider, obje
         /// </summary>
         private static void CheckDefaultProvider(Type type)
         {
+            bool providerAdded = false;
+
             if (s_defaultProviders.ContainsKey(type))
             {
                 return;
             }
 
-            lock (s_internalSyncObject)
-            {
-                if (s_defaultProviders.ContainsKey(type))
-                {
-                    return;
-                }
-
-                // 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;
-            }
-
             // 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.
+            // 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];
                 Type? providerType = Type.GetType(pa.TypeName);
                 if (providerType != null && typeof(TypeDescriptionProvider).IsAssignableFrom(providerType))
                 {
                     TypeDescriptionProvider prov = (TypeDescriptionProvider)Activator.CreateInstance(providerType)!;
-                    AddProvider(prov, type);
+                    AddProvider_FewerLock(prov, type);
                     providerAdded = true;
                 }
             }
 
+            lock (s_internalSyncObject)
+            {
+                s_defaultProviders[type] = null;
+            }
+
             // If we did not add a provider, check the base class.
             if (!providerAdded)
             {
@@ -1500,6 +1517,45 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator)
             return node;
         }
 
+        /// <summary>
+        /// Specialized version for calls from <see cref="CheckDefaultProvider(Type)"/> and <see cref="AddProvider_FewerLock(TypeDescriptionProvider, Type)"/>.
+        /// Code is adjusted to minimize lock.
+        /// </summary>
+        /// <param name="type"></param>
+        /// <returns></returns>
+        private static TypeDescriptionNode NodeFor_LockFreeWithCreateDelegator(Type type)
+        {
+            // lock is in the outer method.
+
+            var node = (TypeDescriptionNode?)(s_providerTypeTable[type] ?? s_providerTable[type]);
+            if (node != null)
+            {
+                return node;
+            }
+
+            Type? baseType = GetNodeForBaseType(type);
+
+            if (type == typeof(object) || baseType == null)
+            {
+                node = (TypeDescriptionNode?)s_providerTable[type];
+
+                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[type] = node;
+                }
+            }
+            else
+            {
+                node = new TypeDescriptionNode(new DelegatingTypeDescriptionProvider(baseType));
+                s_providerTypeTable[type] = node;
+            }
+
+            return node;
+        }
+
         /// <summary>
         /// Retrieves the head type description node for an instance.
         /// Instance-based node lists are rare. If a node list is not

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

@@ -0,0 +1,110 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace System.ComponentModel.Tests
+{
+    [Collection(nameof(DisableParallelization))] // manipulates cache
+    public class ConcurrentTypeDescriptorTests
+    {
+        private long _error = 0;
+        private bool Error
+        {
+            get => Interlocked.Read(ref _error) == 1;
+            set => Interlocked.Exchange(ref _error, value ? 1 : 0);
+        }
+
+        private void ConcurrentTest(TypeWithProperty instance)
+        {
+            var properties = TypeDescriptor.GetProperties(instance);
+            Thread.Sleep(10);
+            if (properties.Count > 0)
+            {
+                Error = true;
+            }
+        }
+
+        [Fact]
+        public void GetProperties_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(Error, "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; }
+        }
+    }
+}