Can anonymous event handler in attached behavior cause leak?

Question

I have following code.

So basically it executes command (DelegateCommand based on weak reference delegates), when Selector.SelectionChanged event is raised.

    public static readonly DependencyProperty SelectionCommandProperty
        = DependencyProperty.RegisterAttached(
            "SelectionCommand",
            typeof(ICommand),
            typeof(CommonUtilities),
            new PropertyMetadata(null, OnSelectionCommandPropertyChanged));

    private static void OnSelectionCommandPropertyChanged(
        DependencyObject d, DependencyPropertyChangedEventArgs e)
    {
        var selector = d as Selector;
        var command = e.NewValue as ICommand;
        if (selector != null && command != null)
        {
            selector.SelectionChanged
                += (o, args) => command.Execute(selector.SelectedItem);
        }
    }

    public static ICommand GetSelectionCommand(DependencyObject d)
    {
        return d.GetValue(SelectionCommandProperty) as ICommand;
    }

    public static void SetSelectionCommand(DependencyObject d, ICommand value)
    {
        d.SetValue(SelectionCommandProperty, value);
    }

Note that the context is static.

Does this cause leak? I can guess that it doesnt because as far as I know, the anonymous handler would be in effect until the scope of all "outer" variables (i.e. selector, command here) is not applicable for GC. Once they are GCed which would happen when the View (that has selector) and ViewModel (that is supplying command) are unloaded from parent GUI, the anonymous delegate would also be unhooked.

Am I right here?

Lukazoid · Accepted Answer · 2012-12-13T13:18:56.537

Here are the references in this example:

View:
- Selector
ViewModel:
- ICommand
Selector:
- Anonymous delegate
- ICommand
Anonymous delegate:
- Selector
- ICommand

This means the view and view-model can be garbage collected, leaving the Selector and ICommand alive.

The garbage collector is capable of dealing with circular references; so even though the Selector references the delegate, and the delegate references the Selector these can still be garbage collected.

The ICommand however, will be kept alive as long as this anonymous delegate is kept alive, which is determined solely by the lifetime of the Selector instance. As long as the Selector is being garbage collected, the delegate and ICommand will eventually be garbage collected too.

So in simple situations, no, your code does not cause a leak.

There is however a situation where your code does leak handlers, I am assuming your view-model has a property like so:

public ICommand OnSelectionChanged
{
    get { return _onSelectionChanged; }
    private set 
    { 
        _onSelectionChanged = value;
        RaisePropertyChanged("OnSelectionChanged");
    }
}

Which is then bound in the view, if you change the value of this OnSelectionChanged command, your attached property will leak event handlers, as you never unsubscribe the delegate which executes the old command.

So rather than just one command being executed, all of the previous values of this property will be executed.

I would go for an implementation more like the following:

private static void OnSelectionCommandPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
    var selector = d as Selector;

    if (selector != null)
    {
        var oldCommand = e.OldValue as ICommand;
        var newCommand = e.NewValue as ICommand;
        if(oldCommand == null && newCommand != null)
        {
            selector.SelectionChanged += OnSelectionChanged;
        }
        else
        {
            selector.SelectionChanged -= OnSelectionChanged;
        }
    }
}

private static void OnSelectionChanged(object sender, SelectionChangedEventArgs e)
{
    var selector = (Selector)sender;
    var command = GetSelectionCommand(selector);
    if(command != null)
    {
        command.Execute(selector.SelectedItem);
    }
}

fantastic answer. Yes I make sure the pattern of loading the `ICommand` is once only i.e. there is no reason why I need to raise a property changed notification for it. The private variable in my `ViewModel` that holds the command is `readonly` and is loaded at the creation of the VM instance. Plus the command itself implements weak delegates for `CanExecute` and `Execute`. — WPF-it, Dec 13 '12 at 13:21

score 0 · Answer 2 · answered Dec 13 '12 at 13:12

Lifetime of the anonymous handler depends strictly on your selector object, not on the outer variables, so it will exist until you unsubscribe or the selector object is garbage-collected. So in this way it can't cause a memory leak.

Please note that there may be multiple subscriptions for the same objects, so you may need to unsubscribe at some point.

Can anonymous event handler in attached behavior cause leak?

2 Answers2