From 4c06b40d5ac0a9f223c065f4d5a85d197fbfacb8 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 19 Jan 2024 16:18:37 -0600 Subject: [PATCH 1/2] Avoid deadlock by sharing a lock object --- .../System/ComponentModel/TypeDescriptor.cs | 12 +++-- .../tests/TypeDescriptorTests.cs | 52 +++++++++++++++++++ 2 files changed, 59 insertions(+), 5 deletions(-) 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..8000a1d04f433 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,57 @@ 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 + }; + + ConcurrentAddProvider(); + + 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() + { + TypeDescriptor.AddProvider(new EmptyPropertiesTypeProvider(), typeof(MyClass)); + } + + static void ConcurrentGetProvider() + { + TypeDescriptor.GetProvider(typeof(TypeWithProperty)); + } + } + public sealed class EmptyPropertiesTypeProvider : TypeDescriptionProvider { private sealed class EmptyPropertyListDescriptor : ICustomTypeDescriptor From 292e7c7733eb38634136f41a43205e73f99b7f32 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Sat, 20 Jan 2024 11:19:44 -0600 Subject: [PATCH 2/2] Remove LOC that was used for debugging --- .../tests/TypeDescriptorTests.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs index 8000a1d04f433..b5468a6583f46 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs @@ -1302,8 +1302,6 @@ public static void ConcurrentAddProviderAndGetProvider() TimeOut = 60000 }; - ConcurrentAddProvider(); - RemoteExecutor.Invoke(() => { using var finished = new CountdownEvent(2); @@ -1332,12 +1330,19 @@ public static void ConcurrentAddProviderAndGetProvider() static void ConcurrentAddProvider() { - TypeDescriptor.AddProvider(new EmptyPropertiesTypeProvider(), typeof(MyClass)); + 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() { - TypeDescriptor.GetProvider(typeof(TypeWithProperty)); + TypeDescriptionProvider provider = TypeDescriptor.GetProvider(typeof(TypeWithProperty)); + + // This test primarily verifies no deadlock, but verify the values anyway. + Assert.True(provider.IsSupportedType(typeof(TypeWithProperty))); } } @@ -1395,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)); }