TypeDescriptor caching blocks unloading of code which uses it from unloadable load context

[vitek-karas]

The implementation of TypeDescription and TypeConverter uses internal caches which hold strong references to the Type object. These remain even after the descriptors and converters are released by the user code.

If these types come from collectible assemblies (either dynamic types, or custom unloadable AssemblyLoadContext), this prevents unloading of such assembly.

This is exaggerated by the fact that TypeConverter is used by Newtonsoft.Json during serilization or deserialization of JSON. This is VERY common in user code. This effectively means that any app which uses plugins and wants to be able to unload the plugins successfully would have to prevent all such plugins from using Newtonsoft.Json serialization on objects from the plugin.

This is basically a feature ask to modify all the internal caches in TypeDescriptor/TypeConverter implementation to be based on weak references, so that it's possible to collect the types.

Customer issue which ran into this, with detailed analysis of what happens: https://github.com/dotnet/coreclr/issues/26271

It was possible to workaround in this case by loading the ComponentModel assemblies many times into separate ALCs, but that comes with large performance penalty and is not an obvious solution to the problem.

[vitek-karas]

/cc @janvorli

[Wraith2]

Would it make sense to make the caches ALC specific so that converters caches in each context don't leak and wait for collection to occur? [edit] I've just realised that they've internal to a third party assembly so that might not be the best solution but it might still be possible.

[vitek-karas]

That could also work - not sure which solution is best for perf...
Currently there's no framework-wide mechanism to attach something to ALC, so it would have to be a custom solution (basically a weak hash table of all ALCs). It's kind of interesting because the TypeDescriptor.s_providerTable is already a WeakHashtable and couple of others are as well, but not all. I don't know the history of this code, why it was done this way.

[tarekgh]

@vitek-karas I assume this issue is not meeting the 3.0 bar so I marked it as for 5.0 release. Let me know if this is not the case.

[vitek-karas]

I agree it does not meet 3.0, but I think we should at least consider it for 3.1 - it blocks lot of unloadability scenarios - with no good workaround.

[Wraith2]

Just having a look through the code this change might be as simple as changing some Hashtable to WeakHashtable but it's going to have some performance degradations under load. There are also a number of places where WeakReference.IsAlive is used which I know is now suggested against so could be changed at the same time.

[maryamariyan]

Talked offline with @vitek-karas and here is a possible action plan forward for this issue:

• Possibly a simple fix is to replace one of the strong references with a weak reference. But we also want to check for perf regressions and another concern is for cases when GC after the fix clears the cache.

• Also note, a sample repro for this issue is in description of https://github.com/dotnet/coreclr/issues/26271

[ericstj]

Not seeing this meet the bar for 5.0 at this point either. We haven't seen a ton of interest in this outside of this report. The risk here is substantial as this code has been around for decades now and is a critical piece of windows App models like Winforms and WPF. We'd need to be very careful when touching this not to change behavior nor impact perf substantially. We're not prepared to take that amount of risk at this point in the product.