From 798d47952c823b8b361fb1054ff07efd64dddd00 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 21 Jun 2024 13:37:10 -0500 Subject: [PATCH] Avoid rare deadlocks when using TypeDescriptor --- .../ReflectTypeDescriptionProvider.cs | 23 +++++++------- .../System/ComponentModel/TypeDescriptor.cs | 30 +++++++++++-------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs index 28beac41a7c2fa..531989154e4c6a 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs @@ -82,7 +82,6 @@ private static Type[] InitializeSkipInterfaceAttributeList() internal static Guid ExtenderProviderKey { get; } = Guid.NewGuid(); - private static readonly object s_internalSyncObject = new object(); /// /// Creates a new ReflectTypeDescriptionProvider. The type is the /// type we will obtain type information for. @@ -224,7 +223,7 @@ internal static void AddEditorTable(Type editorBaseType, Hashtable table) Debug.Assert(table != null, "COMPAT: Editor table should not be null"); // don't throw; RTM didn't so we can't do it either. - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { Hashtable editorTables = EditorTables; if (!editorTables.ContainsKey(editorBaseType)) @@ -436,7 +435,7 @@ internal TypeConverter GetConverterFromRegisteredType(Type type, object? instanc // if (table == null) { - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { table = editorTables[editorBaseType]; if (table == null) @@ -863,7 +862,7 @@ internal Type[] GetPopulatedTypes(Module module) { List typeList = new List(); - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { Dictionary? typeData = _typeData; if (typeData != null) @@ -936,7 +935,7 @@ public override Type GetReflectionType( return td; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { if (_typeData != null && _typeData.TryGetValue(type, out td)) { @@ -999,7 +998,7 @@ private ReflectedTypeData GetTypeDataFromRegisteredType(Type type) return; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { if (_typeData != null && _typeData.ContainsKey(componentType)) { @@ -1024,7 +1023,7 @@ private ReflectedTypeData GetOrRegisterType(Type type) return td; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { if (_typeData != null && _typeData.TryGetValue(type, out td)) { @@ -1122,7 +1121,7 @@ internal static Attribute[] ReflectGetAttributes(Type type) return attrs; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { attrs = (Attribute[]?)attributeCache[type]; if (attrs == null) @@ -1150,7 +1149,7 @@ internal static Attribute[] ReflectGetAttributes(MemberInfo member) return attrs; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { attrs = (Attribute[]?)attributeCache[member]; if (attrs == null) @@ -1178,7 +1177,7 @@ private static EventDescriptor[] ReflectGetEvents(Type type) return events; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { events = (EventDescriptor[]?)eventCache[type]; if (events == null) @@ -1275,7 +1274,7 @@ private static PropertyDescriptor[] ReflectGetExtendedProperties(IExtenderProvid ReflectPropertyDescriptor[]? extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType]; if (extendedProperties == null) { - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { extendedProperties = (ReflectPropertyDescriptor[]?)extendedPropertyCache[providerType]; @@ -1363,7 +1362,7 @@ private static PropertyDescriptor[] ReflectGetPropertiesImpl(Type type) return properties; } - lock (s_internalSyncObject) + lock (TypeDescriptor.s_commonSyncObject) { properties = (PropertyDescriptor[]?)propertyCache[type]; 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 9cab702218a067..78f34efbfb73a6 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs @@ -48,6 +48,13 @@ public sealed class TypeDescriptor // class load anyway. private static readonly WeakHashtable s_providerTable = new WeakHashtable(); + // This lock object protects access to several thread-unsafe areas below, and is a single lock object to prevent deadlocks. + // - During s_providerTypeTable access. + // - To act as a mutex for CheckDefaultProvider() when it needs to create the default provider, which may re-enter the above case. + // - For cache access in the ReflectTypeDescriptionProvider class which may re-enter the above case. + // - For logic added by consumers, such as custom provider, constructor and property logic, which may re-enter the above cases in unexpected ways. + internal static readonly object s_commonSyncObject = new object(); + // A direct mapping from type to provider. private static readonly Dictionary s_providerTypeTable = new Dictionary(); @@ -240,7 +247,7 @@ public static void AddProvider(TypeDescriptionProvider provider, Type type) ArgumentNullException.ThrowIfNull(provider); ArgumentNullException.ThrowIfNull(type); - lock (s_providerTable) + lock (s_commonSyncObject) { // Get the root node, hook it up, and stuff it back into // the provider cache. @@ -270,7 +277,7 @@ public static void AddProvider(TypeDescriptionProvider provider, object instance // Get the root node, hook it up, and stuff it back into // the provider cache. - lock (s_providerTable) + lock (s_commonSyncObject) { refreshNeeded = s_providerTable.ContainsKey(instance); TypeDescriptionNode node = NodeFor(instance, true); @@ -331,10 +338,7 @@ private static void CheckDefaultProvider(Type type) return; } - // 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) + lock (s_commonSyncObject) { AddDefaultProvider(type); } @@ -342,7 +346,7 @@ private static void CheckDefaultProvider(Type type) /// /// Add the default provider, if it exists. - /// For threading, this is always called under a 'lock (s_providerTable)'. + /// For threading, this is always called under a 'lock (s_commonSyncObject)'. /// private static void AddDefaultProvider(Type type) { @@ -1666,7 +1670,7 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator) if (searchType == typeof(object) || baseType == null) { - lock (s_providerTable) + lock (s_commonSyncObject) { node = (TypeDescriptionNode?)s_providerTable[searchType]; @@ -1682,7 +1686,7 @@ private static TypeDescriptionNode NodeFor(Type type, bool createDelegator) else if (createDelegator) { node = new TypeDescriptionNode(new DelegatingTypeDescriptionProvider(baseType)); - lock (s_providerTable) + lock (s_commonSyncObject) { s_providerTypeTable.TryAdd(searchType, node); } @@ -1793,7 +1797,7 @@ private static TypeDescriptionNode NodeFor(object instance, bool createDelegator /// private static void NodeRemove(object key, TypeDescriptionProvider provider) { - lock (s_providerTable) + lock (s_commonSyncObject) { TypeDescriptionNode? head = (TypeDescriptionNode?)s_providerTable[key]; TypeDescriptionNode? target = head; @@ -2314,7 +2318,7 @@ private static void Refresh(object component, bool refreshReflectionProvider) { Type type = component.GetType(); - lock (s_providerTable) + lock (s_commonSyncObject) { // ReflectTypeDescritionProvider is only bound to object, but we // need go to through the entire table to try to find custom @@ -2398,7 +2402,7 @@ public static void Refresh(Type type) bool found = false; - lock (s_providerTable) + lock (s_commonSyncObject) { // ReflectTypeDescritionProvider is only bound to object, but we // need go to through the entire table to try to find custom @@ -2463,7 +2467,7 @@ public static void Refresh(Module module) // each of these levels. Hashtable? refreshedTypes = null; - lock (s_providerTable) + lock (s_commonSyncObject) { // Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations. IDictionaryEnumerator e = s_providerTable.GetEnumerator();