Skip to content

Avoid rare deadlocks when using TypeDescriptor #103835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Avoid rare deadlocks when using TypeDescriptor #103835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions ....ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ private static Type[] InitializeSkipInterfaceAttributeList()

internal static Guid ExtenderProviderKey { get; } = Guid.NewGuid();

private static readonly object s_internalSyncObject = new object();
/// <summary>
/// Creates a new ReflectTypeDescriptionProvider. The type is the
/// type we will obtain type information for.
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -863,7 +862,7 @@ internal Type[] GetPopulatedTypes(Module module)
{
List<Type> typeList = new List<Type>();

lock (s_internalSyncObject)
lock (TypeDescriptor.s_commonSyncObject)
{
Dictionary<Type, ReflectedTypeData>? typeData = _typeData;
if (typeData != null)
Expand Down Expand Up @@ -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))
{
Expand Down Expand Up @@ -999,7 +998,7 @@ private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)
return;
}

lock (s_internalSyncObject)
lock (TypeDescriptor.s_commonSyncObject)
{
if (_typeData != null && _typeData.ContainsKey(componentType))
{
Expand All @@ -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))
{
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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];

Expand Down Expand Up @@ -1363,7 +1362,7 @@ private static PropertyDescriptor[] ReflectGetPropertiesImpl(Type type)
return properties;
}

lock (s_internalSyncObject)
lock (TypeDescriptor.s_commonSyncObject)
{
properties = (PropertyDescriptor[]?)propertyCache[type];

Expand Down
30 changes: 17 additions & 13 deletions ...libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Type, TypeDescriptionNode> s_providerTypeTable = new Dictionary<Type, TypeDescriptionNode>();

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -331,18 +338,15 @@ 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);
}
}

/// <summary>
/// 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)'.
/// </summary>
private static void AddDefaultProvider(Type type)
{
Expand Down Expand Up @@ -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];

Expand All @@ -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);
}
Expand Down Expand Up @@ -1793,7 +1797,7 @@ private static TypeDescriptionNode NodeFor(object instance, bool createDelegator
/// </summary>
private static void NodeRemove(object key, TypeDescriptionProvider provider)
{
lock (s_providerTable)
lock (s_commonSyncObject)
{
TypeDescriptionNode? head = (TypeDescriptionNode?)s_providerTable[key];
TypeDescriptionNode? target = head;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
Loading