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 6043b69b4f80f..948e3f4b386f3 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs @@ -33,7 +33,6 @@ public sealed class TypeDescriptor // 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. @@ -271,7 +270,10 @@ private static void CheckDefaultProvider(Type type) return; } - lock (s_defaultProviderSyncObject) + // 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) { AddDefaultProvider(type); } @@ -279,7 +281,7 @@ private static void CheckDefaultProvider(Type type) /// /// Add the default provider, if it exists. - /// For threading, this is always called under a 'lock (s_defaultProviderSyncObject)'. + /// For threading, this is always called under a 'lock (s_providerTable)'. /// private static void AddDefaultProvider(Type type) { @@ -326,11 +328,11 @@ private static void AddDefaultProvider(Type type) } /// - /// 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. /// [EditorBrowsable(EditorBrowsableState.Advanced)] diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs index 95bac7b0ba892..b5468a6583f46 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs @@ -8,6 +8,7 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; +using Microsoft.DotNet.RemoteExecutor; using Moq; using Xunit; @@ -1289,6 +1290,62 @@ public void ConcurrentGetProperties_ReturnsExpected() } } + [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 @@ -1343,6 +1400,7 @@ public async void GetConverter_ByMultithread_ReturnsExpected(Type typeForGetConv TypeConverter[] actualConverters = await Task.WhenAll( Enumerable.Range(0, 100).Select(_ => Task.Run(() => TypeDescriptor.GetConverter(typeForGetConverter)))); + Assert.All(actualConverters, currentConverter => Assert.IsType(expectedConverterType, currentConverter)); }