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..680bd87f12d717 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs @@ -189,6 +189,30 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type) Refresh(type); } + /// + /// Specialized version for calls from . + /// Code is adjusted to minimize lock in the aforementioned method. + /// + /// + /// + 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); + } + /// /// Adds a type description provider that will be called on to provide /// type information for a single object instance. A provider added @@ -262,32 +286,20 @@ public static void AddProviderTransparent(TypeDescriptionProvider provider, obje /// 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]; @@ -295,11 +307,16 @@ private static void CheckDefaultProvider(Type type) 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; } + /// + /// Specialized version for calls from and . + /// Code is adjusted to minimize lock. + /// + /// + /// + 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; + } + /// /// Retrieves the head type description node for an instance. /// Instance-based node lists are rare. If a node list is not diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs new file mode 100644 index 00000000000000..2d125e7d261d1f --- /dev/null +++ b/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; } + } + } +}