dennisdoomen / dennisdoomen/reflectify

[Feature]: The static Reflector cache is unbounded and pins assemblies, preventing AssemblyLoadContext unload

Open
#163 0 comments 0 reactions 0 assignees View on GitHub
feature triage
Dominant language
C#
Stars
80
Forks
6
Avg merge
1d 12m
Merged PRs (30d)
11

Description

### Background and motivation

`TypeMemberExtensions` caches every reflected type forever:

```csharp
private static readonly ConcurrentDictionary<(Type Type, MemberKind Kind), Reflector> ReflectorCache = new();

private static Reflector GetFor(Type typeToReflect, MemberKind kind)
{
return ReflectorCache.GetOrAdd((typeToReflect, kind),
static key => new Reflector(key.Type, key.Kind));
}
```

This is the right default for performance and there is a `PerformanceSpecs` test protecting it. But there are two consequences that are not currently addressed.

**1. It keeps assemblies alive.** The dictionary is `static readonly` and holds `Type` keys plus `PropertyInfo`/`FieldInfo` arrays. A `Type` roots its `Assembly`, which roots its `AssemblyLoadContext`. In any host that loads and unloads assemblies (plugin systems, add-in models, test runners that build dynamic assemblies, anything using a collectible `AssemblyLoadContext`), a single Reflectify call permanently prevents unload. Because Reflectify is a source-only package compiled into the consumer, the leak is attributed to the consumer's assembly and is hard to diagnose.

**2. It grows without bound.** The key includes `MemberKind`, so the same type can be cached under many different flag combinations. For dynamically generated types the growth is unbounded.

This is not hypothetical for the library's largest known consumer: assertion libraries frequently run inside test hosts that generate proxy types.

### Alternative Concerns

- **A `ConditionalWeakTable` keyed on `Type` instead of a `ConcurrentDictionary`.** This solves unloadability properly, since entries die with the type. The complication is the composite `(Type, MemberKind)` key: it would need to be a table of `Type` to a small per-kind map. Slightly more allocation, but it removes the leak without any API surface at all, which makes it the most attractive option.
- **A public `ReflectionCache.Clear()` hook.** Simple and explicit, and it lets a plugin host clear on unload. Downside: it is opt-in, so it only helps people who already know the problem exists.
- **A bounded cache with eviction.** Adds policy and complexity for a library that values being small and predictable. Probably not worth it.
- **Document it and do nothing.** Legitimate, given the performance goal, but it leaves a real footgun in a package explicitly designed to be embedded in *other* libraries, whose own consumers never chose Reflectify.

Note there is a related smaller issue in the same area: `Reflector` uses double-checked locking with a shared `lazyLoadingLock` for both `Properties` and `Fields`, so loading fields blocks loading properties for the same type. Harmless, but a separate lock per field would be trivial.

Whatever is decided, the README should state the caching behaviour. It currently does not mention it at all, so consumers cannot even make an informed choice.

### Are you willing help with a pull-request?

No

Contributor guide

Open the contributing guide

Research direction

Start with TypeMemberExtensions and Reflector, then read the PerformanceSpecs test to understand the current cache contract and performance expectations. Compare the cache alternatives described in the issue, account for collectible AssemblyLoadContext unloadability, and update the README with the resulting caching behavior. Done means the chosen behavior is implemented, covered by relevant tests, and documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.