Concurrency issue in TypeDescriptor.GetConverter(type)

[PawelGerr]

TypeDescriptor.GetConverter(type) returns different results when calling on multiple threads with the same type and this type has a custom TypeDescriptionProvider.

This issue is reproducible with target frameworks netcoreapp2.2 and net472 on window 10 x64.

Repro

I'm calling the method on 10 threads and print the fetched type converter at the end.

Expected output

All 10 calls return MyTypeConverter

Actual output

The method return both TypeConverter and MyTypeConverter in random order like the following:

TypeConverter
TypeConverter
TypeConverter
MyTypeConverter
TypeConverter
TypeConverter
TypeConverter
TypeConverter
TypeConverter
TypeConverter

The good thing is that the method is returning MyTypeConverter after the concurrent calls are finished. The bad thing is that there are libs (like newtonsoft.json) that are caching the type descriptors. If the cache of 3rd party lib is initialized with wrong descriptor then the application is broken until restart.

Program.cs

public class Program
{
   public static async Task Main()
   {
      var tasks = new Task<TypeConverter>[10];

      for (var i = 0; i < tasks.Length; i++)
      {
         tasks[i] = new Task<TypeConverter>(() => TypeDescriptor.GetConverter(typeof(MyClass)));
      }

      foreach (var t in tasks)
      {
         t.Start();
      }

      foreach (var conv in await Task.WhenAll(tasks))
      {
         Console.WriteLine(conv.GetType().Name);
      }
   }
}

[TypeDescriptionProvider(typeof(MyClassTypeDescriptionProvider))]
public class MyClass
{
}

public class MyClassTypeDescriptionProvider : TypeDescriptionProvider
{
   public override ICustomTypeDescriptor GetTypeDescriptor(Type objectType, object instance)
   {
      return new MyClassTypeDescriptor();
   }
}

public class MyClassTypeDescriptor : CustomTypeDescriptor
{
   public override TypeConverter GetConverter()
   {
      return new MyTypeConverter();
   }
}

public class MyTypeConverter : TypeConverter
{
}

.csproj-file

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <TargetFramework>netcoreapp2.2</TargetFramework>
        <OutputType>EXE</OutputType>
    </PropertyGroup>
</Project>

[stephentoub]

The bug here is in CheckDefaultProvider. It appears its purpose is to examine a type and add appropriate providers, and it uses a set to indicate whether a type has already been examined... but it marks the type examined at the beginning of its execution rather than at the end, such that any threads that then concurrently look up the provider for that type miss the default provider until it completes. It's not just as simple, however, as moving the marking to the end, as it's also trying to work around a recursive usage.

[safern]

@stephentoub given that you self-assigned this, you plan to fix this before we branch tomorrow? I want to know what is your timeframe for this in order to triage accordingly.

[stephentoub]

Been staring at it for a bit, and it doesn't look like there's a trivial fix here. This issue has been here basically forever as far as I can tell, so while it'd be good to fix it, I think it's best not to try to rush a fix into 3.0.

[safern]

Thanks for confirming, @stephentoub

[eerhardt]

Moving to Future as this issue has been around for a long time and is not critical to fix in 7.0.

[ericstj]

Adding some links based on @stephentoub's analysis above.

The issue stems from this

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

Lines 270 to 281 in cfa42d3

	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;
	}

Which allows a second caller to escape this function before the first thread's initialization completes.

My first thought was to hold onto the s_internalSyncObject longer - until the end of this method. That would require us to also acquire that lock before the first check though - which could introduce a performance penalty in the initialized case.

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

Lines 265 to 268 in cfa42d3

	if (s_defaultProviders.ContainsKey(type))
	{
	return;
	}

I don't think we'd want to introduce such a penalty - this code-path seems to have been intentional about avoiding a lock in the read case.

Perhaps a way to solve this, retaining the protection for recursion and avoiding a performance penalty (or another list) is to use a sentinel value during initialization - in addition to holding the initialization lock for the duration of the initialization phase.

So we can have public static object s_initializingProvider = new object();

We'll set this as the value for type instead of null during initialization. When we check for the value outside the lock we'll only return if we get a value that is not the sentinel value. If we find the sentinel value (or no value at all) we'll enter the lock and do the initialization. Under the lock if we find any value (including the sentinel) we'd just return since it means we're either in the recursive case or some other thread completed initialization.

@stephentoub does this sound like it would work? If so I can propose a PR.

[ericstj]

I just noticed that #92521 was also trying to fix this by adding an extra hashtable. That works, but I wonder if we can solve it with just a sentinel. @karakasa can you have a read of my suggestion above and see if it makes sense?

[stephentoub]

@stephentoub does this sound like it would work? If so I can propose a PR.

It's been a while since I looked at this, but at first glance your suggestion sounds viable.

[steveharter]

The sentinel pattern was applied new PR created: #96846

[steveharter]

Long-standing threading issues in TypeDescriptor have been addressed in .NET 9 Preview 1, so any existing work-arounds can be removed. Workarounds included pre-populating internal caches such as by calling TypeDescriptor.GetProvider(type) for every type that has a TypeProviderAttribute, and by wrapping the access to the affected APIs with a lock statement.

Also see TypeDescriptor.GetProperties(object instance) is not thread-safe.

The threading issues also exist in .NET Framework. If you are affected by this issue in .NET Framework, please add feedback here so we can determine the priority of porting the fix to .NET Framework.