MahApps / MahApps/MahApps.Metro
DialogParticipation registrations are never released (ConditionalWeakTable proposal)
- Dominant language
- C#
- Stars
- 9.8k
- Forks
- 2.4k
- Avg merge
- 1h 2m
- Merged PRs (30d)
- 56
Description
Follow-up to #2241, which described this and was closed with the advice to unregister manually. That workaround still works and is now written down in the [MVVM dialog docs](https://mahapps.github.io/mahapps.com/docs/dialogs/mvvm-dialog), but the underlying behaviour is unchanged, still reachable by default, and was raised again in that thread. Opening a separate issue for the structural fix so the discussion is not buried in a closed one.
## Describe the bug
`DialogParticipation` keeps its registrations in a static dictionary:
```csharp
private static readonly IDictionary ContextRegistrationIndex
= new Dictionary();
```
Both the key (the context, normally a view model) and the value (the element it was registered on) are held by strong references. An entry is removed only when the attached property *changes* — closing a window does not change it. A window that is opened repeatedly therefore leaves one view model and one element behind on every round, for the lifetime of the process.
## Steps to reproduce
Open and close a `MetroWindow` that carries `mah:DialogParticipation.Register="{Binding}"` a few times, then look at `DialogParticipation`'s registration index.
Measured against 2.4.11:
| | entries |
| --- | --- |
| 3 windows opened and closed, no unregister | 0 → 3 |
| 3 windows opened and closed, `SetRegister(this, null)` on `Closed` | 3 → 3 |
| 3 more, no unregister | 3 → 6 |
So nothing is reclaimed unless the caller remembers to clear the property. The same code is on `develop`, so v3 behaves the same way.
## Expected behavior
A registration should not outlive the thing it was registered on. Forgetting to unregister should cost nothing.
## Actual behavior
Every registration is permanent until explicitly overwritten or cleared.
## Suggestion
`ConditionalWeakTable` instead of `Dictionary`.
The reason a plain `WeakReference` is not enough here is the shape of the data: the value references the key. The registered element is usually the window, and the window's `DataContext` is the context that serves as the key, so a weak key with a strong value would keep itself alive. `ConditionalWeakTable` has ephemeron semantics and handles exactly that cycle — the entry survives only while the key is reachable from somewhere else.
Confirmed with a small probe on the same object graph (value holds a reference back to the key), after forcing a collection:
```
Dictionary -> key alive: True value alive: True
ConditionalWeakTable -> key alive: False value alive: False
```
The four usages map over without changing the public API:
| today | with `ConditionalWeakTable` |
| --- | --- |
| `ContextRegistrationIndex.Remove(oldValue)` | `Remove(oldValue)` |
| `ContextRegistrationIndex[newValue] = element` | `Remove(newValue)` then `Add(newValue, element)` |
| `ContextRegistrationIndex.ContainsKey(context)` | `TryGetValue(context, out _)` |
| `ContextRegistrationIndex[context]` | `TryGetValue(context, out var element)` |
Two things to be aware of:
- `AddOrUpdate` is not available on `net452`, which the library still targets, so the assignment needs `Remove` followed by `Add`.
- `ConditionalWeakTable` compares keys by reference, whereas `Dictionary` uses the default comparer. A context that overrides `Equals`/`GetHashCode` would be treated differently. In practice a view model is matched by identity anyway, and `GetAssociation` is only ever called with the same instance that was registered, but it is a behavioural difference worth naming.
The alternative raised in #2241 — having the attached property subscribe to `Unloaded` and unregister itself — removes the need for callers to remember, which is the practical half of the problem. It does not remove the strong references, so a context whose element is never unloaded still stays. The two are not exclusive.
## Environment
```
MahApps.Metro version: v2.4.11 (and develop)
Target Framework: net8.0-windows
```
Contributor guide
Research direction
Locate DialogParticipation and inspect the static ContextRegistrationIndex plus its four usages. Reproduce the repeated MetroWindow open/close case, then verify the registration storage remains compatible with net452 and no longer keeps unreachable contexts and elements alive without manual unregistering.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- desktop
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100