Use AssemblyLoadContext-aware caches in TypeDescriptor to support unloading of assemblies cached by TypeDescriptor

[alexey-zakharov]

Fixes #30656 - TypeDescriptor caches keeping strong references to the types from collectible assemblies.

Motivation

TypeDescriptor uses static caches to amortize attributes retrieval and store user providers. The caches use Type-to-object approach and capture strong reference to the Type and dependent objects. That also includes ReflectTypeDescriptionProvider) implementation. Strong reference to the Type prevents collectible types and thus collectible assemblies from being collected and unloaded.

Changes

The PR applies the "context aware table" pattern used in coreclr codebase to split caching of collectible and non-collectible types into 2 tables - standard Hashtable/Dictionary for non-collectible types and WeakHashtable/ConditionalWeaktable for collectible types.

• Introduced ContextAwareConcurrentHashtable and ContextAwareHashtable helper classes which leverage DependentHandle/WeakReference for collectible types.
• Used context aware tables TypeDescription and ReflectTypeDescriptionProvider implementations to ensure there are no strong references from static caches to types from unloadable assemblies.

Note that the collectible Type used as a key will be only collected once AssemblyLoadContext.Unload is called since the type instance for collectible types is allocated by LoaderAllocator and stored in its m_slots table for the whole lifetime of LoaderAllocator. Thus all custom collectible providers will be kept alive until the point when their parent AssemblyLoadContext is explicitly unloaded.

The performance impact of the change may be observed for the collectible types - dereferencing DependentHandle and WeakReference adds overhead and may introduce additional entries in the handle tables. And the cost of calling MemberInfo.IsCollectible is added to all queries.

The PR addresses #114620 #30656 issue.

Testing

• Added TypeDescriptorTests.TypeDescriptor_DoesNotKeepUnloadableTypes test to validate caching for collectible types.
• Validated System.ComponentModel.TypeConverter.Tests locally.
• Validated no ALC leaks are present using a custom reporting tool.

[Copilot]

Copilot reviewed 6 out of 9 changed files in this pull request and generated no comments.

Files not reviewed (3)

• src/libraries/System.ComponentModel.TypeConverter/System.ComponentModel.TypeConverter.sln: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/System.ComponentModel.TypeConverter.Tests.csproj: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/UnloadableTestTypes/UnloadableTestTypes.csproj: Language not supported

Comments suppressed due to low confidence (1)

src/libraries/System.ComponentModel.TypeConverter/tests/ReflectionCachesUpdateHandlerTests.cs:104

• Consider increasing the number of garbage collection attempts or using a more robust timeout mechanism to ensure that the assembly unloads reliably across different environments.

for (int i = 0; weakRef.IsAlive && i < 10; i++)

[jkotas]

caches are cleared on AssemblyLoadContext unloading.

MetadataUpdateHandler is not meant to be used to prepare caches for AssemblyLoadContext unloading.

[jkotas]

the potential alternative could be to use WeakHashtable instead. However this would come with performance costs

The simplest way to avoid the overhead is to skip the caching completely for collectible types. We do that in number of places in this repo.

[alexey-zakharov]

The simplest way to avoid the overhead is to skip the caching completely for collectible types. We do that in number of places in this repo.

thanks for the suggestion!
we could skip caching completely, but I would expect potential json serialization performance regression for types from collectible assemblies
I'll update the PR to do this instead

[jkotas]

If the performance regression is not acceptable, it is an option to have two caches - one for collectible types and one for non-collectible types. It avoids the performance overhead of weak handles for non-colllectible types.

[alexey-zakharov]

If the performance regression is not acceptable, it is an option to have two caches - one for collectible types and one for non-collectible types. It avoids the performance overhead of weak handles for non-colllectible types.

@jkotas to clarify - would you be ok with a solution that keeps existing cache for types in non-collectible ALCs and WeakHashtable for the types in collectible ALCs (for both TypeDescriptor and ReflectTypeDescriptionProvider cache levels), correct?

I like this approach, as it gives correctness and performance at a relatively low cost of calling Type.IsCollectible 😄

[steveharter]

I'm not sure on what issue this PR is current solving. Currently it depends on hot reload APIs to be called to clear caches, but that already works for hot reload purposes AFAIK. It doesn't solve the more general issue of unloading a collectible ALC.

[jkotas]

would you be ok with a solution that keeps existing cache for types in non-collectible ALCs and WeakHashtable for the types in collectible ALCs

It would be fine with me. We have similar split in other places.

[alexey-zakharov]

I'm not sure on what issue this PR is current solving. Currently it depends on hot reload APIs to be called to clear caches, but that already works for hot reload purposes AFAIK. It doesn't solve the more general issue of unloading a collectible ALC.

@steveharter yes, I'm going to rework it to split the caches next week.
the reason we went with reusing HotReload api is that the places that we've found leaks so far were matching the MetadataUpdateHandler usages.
I think moving to weak table would remove the need of relying on the MetadataUpdateHandler.

Although there is one case which may need a cache clear approach - ArrayPool<>.Shared holds references to the unloading ALCs in the returned pools. And while it is purged eventually, the purge speed it not sufficient to not run out of memory due to accumulating ALCs. But I'll raise this issue separately - there are multiple possible solutions to consider

[Copilot]

Pull Request Overview

This PR fixes an issue with ReflectTypeDescriptionProvider by ensuring that the cached type is completely removed during cache cleanup, allowing for proper unloading of AssemblyLoadContexts.

• Updated cache data structures to use ContextAwareConcurrentDictionary and ContextAwareHashtable.
• Added new tests to verify that unloadable types are correctly cleared from the cache.
• Refactored related helper classes (WeakHashtable, ContextAwareConcurrentDictionary, and ContextAwareHashtable) for improved cache management.

Reviewed Changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
UnloadableTestTypes.cs	Added sample unloadable type and associated attribute for testing.
TypeDescriptorTests.cs	Introduced new tests to validate cache clearance and proper unloading.
WeakHashtable.cs	Modified weak reference handling for enumeration in caching.
TypeDescriptor.cs	Updated cache implementation to use ContextAware data structures.
ReflectTypeDescriptionProvider.cs	Refactored to use new ContextAwareConcurrentDictionary and improved threading comments.
ContextAwareHashtable.cs	Added new implementation to handle collectible types in caching.
ContextAwareConcurrentDictionary.cs	Introduced new concurrent dictionary to support collectible MemberInfo keys.

Files not reviewed (4)

• src/libraries/System.ComponentModel.TypeConverter/System.ComponentModel.TypeConverter.sln: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/System.ComponentModel.TypeConverter.Tests.csproj: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/UnloadableTestTypes/UnloadableTestTypes.csproj: Language not supported

[Copilot]

Pull Request Overview

This PR fixes a caching issue in ReflectTypeDescriptionProvider by ensuring that collectible types are fully removed from static caches, thereby allowing AssemblyLoadContext unload. Key changes include:

• Transition from ConcurrentDictionary/Hashtable to context-aware collections (ContextAwareConcurrentDictionary and ContextAwareHashtable) for collectible types.
• Updates in TypeDescriptor and ReflectTypeDescriptionProvider to use the new collections.
• Addition of tests to confirm that unloadable types are not retained in the caches.

Reviewed Changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/UnloadableTestTypes/UnloadableTestTypes.cs	Added sample unloadable types for testing.
tests/TypeDescriptorTests.cs	Introduced a new AssemblyLoadContext test to verify cache cleanup.
src/System/ComponentModel/WeakHashtable.cs	Updated weak referencing via indexer override.
src/System/ComponentModel/TypeDescriptor.cs	Switched to ContextAware collections and refined lock comments.
src/System/ComponentModel/ReflectTypeDescriptionProvider.cs	Modified caching to use context-aware collections and improved comments.
src/System/ComponentModel/ContextAwareHashtable.cs	New helper to conditionally use weak references for collectible keys.
src/System/ComponentModel/ContextAwareConcurrentDictionary.cs	New concurrent dictionary variant for handling collectible keys.

Files not reviewed (4)

• src/libraries/System.ComponentModel.TypeConverter/System.ComponentModel.TypeConverter.sln: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/System.ComponentModel.TypeConverter.Tests.csproj: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/UnloadableTestTypes/UnloadableTestTypes.csproj: Language not supported

[alexey-zakharov]

@steveharter @jkotas I've updated the implementation to use WeakHashtable and ContextAware* patterns to have a separate caches for collectible types.

That give us a good balance between functional correctness and performance as:

• WeakReference ensures there are no gaps between AssemblyLoadContext unload and repopulating the cache.
• The best case performance overhead (non-unloadable assemblies) is effectively MemberInfo.IsCollectible call only.

[Copilot]

Pull Request Overview

This PR fixes type caching issues to ensure unloadable assembly types are properly removed from static caches and introduces context-aware collections to hold weak references for collectible types.

• Updated tests to validate that unloadable types are cleared.
• Replaced standard collections with ContextAwareConcurrentDictionary and ContextAwareHashtable in caching logic.
• Added helper classes for weak referencing collectible types.

Reviewed Changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/UnloadableTestTypes/UnloadableTestTypes.cs	Added simple unloadable test types.
tests/TypeDescriptorTests.cs	Added assembly load context and unload test to exercise cache clearing.
src/System/ComponentModel/WeakHashtable.cs	Updated enumerator behavior to capture keys during iteration.
src/System/ComponentModel/TypeDescriptor.cs	Migrated to context-aware collections for caching.
src/System/ComponentModel/ReflectTypeDescriptionProvider.cs	Transitioned to context-aware caches and ensured proper removal criteria.
src/System/ComponentModel/ContextAwareHashtable.cs	Introduced new hashtable for wrapping collectible keys in weak references.
src/System/ComponentModel/ContextAwareConcurrentDictionary.cs	Added a new concurrent dictionary for mapping collectible keys using ConditionalWeakTable.

Files not reviewed (4)

• src/libraries/System.ComponentModel.TypeConverter/System.ComponentModel.TypeConverter.sln: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/System.ComponentModel.TypeConverter.Tests.csproj: Language not supported
• src/libraries/System.ComponentModel.TypeConverter/tests/UnloadableTestTypes/UnloadableTestTypes.csproj: Language not supported

[alexey-zakharov]

@ericstj I've addressed your comments, thanks for the review once more!

Main change - Make WeakHashTtable a wrapper around ConditionalWeakTable:
I've found that we need to update main s_providerTable to either wrap ConditionalWeakTable or use DependentHandle for the key/value since the value must be collectible too, but only if the key is collectible. I figured that using ConditionalWeakTable would be a better option since reimplementing ConditionalWeakTable to just avoid locks didn't feel right.

[ericstj]

Thank you for addressing the feedback. This is looking good, just one minor renaming suggestion for consistency.

[alexey-zakharov]

Thank you for addressing the feedback. This is looking good, just one minor renaming suggestion for consistency.

awesome, thanks for the help with the PR!

[alexey-zakharov]

@ericstj do you have any more suggestions? should the PR be merged? 😄

[ericstj]

Sorry for taking a while in getting back to you, I missed the notification on this.

[ericstj]

@alexey-zakharov do you need this in net10.0 or is main sufficient?

[alexey-zakharov]

thanks for merging @ericstj !

@alexey-zakharov do you need this in net10.0 or is main sufficient?

backporting to net10.0 would be ideal as we were planning to upgrade to it
however it is a bit risky change, so perhaps main is good for now and if there are no unforeseen issues with the changes we could backport to 10, what do you think?

[ericstj]

I would definitely like to see some bake time for this change before it makes it to 10.0 - the challenge is that the bar for 10.0 will only be getting higher from here on. @vitek-karas do you have opinions on this? How can we gain confidence in this fix in the scenarios that matter for #30656?

[vitek-karas]

I agree that this change is not the riskier side - mostly because it's used under the hood by many components and potential issues might take a while to show up.