MahApps / MahApps/MahApps.Metro

DialogParticipation registrations are never released (ConditionalWeakTable proposal)

Open
#4,573 0 comments 0 reactions 0 assignees View on GitHub
Bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.