How do I remove event handlers when I'm finish

Question

In my application, I am often creating new Views and ViewModels, but persisting the same Models. For example, I might show a simple view of a list of items in my main window, and have another window with further details of any particular item. The detail window can be opened and closed at any time, or multiple windows can be opened simultaneously for different items on the list.

Therefore, there can be more than one ViewModel for a given model object, and they need to be updated with changes from other places. (I'm using INotifyPropertyChanged on my models.) I want to get rid of ViewModels when I am done with them, i.e., as the detail window is closed.

public DetailViewModel(MyDetailModel detailModel)
{
    // Retain the Detail Model
    this.model = detailModel;

    // Handle changes to the Model not coming from this ViewModel
    this.model.PropertyChanged += model_PropertyChanged;  // Potential leak?
}

It is my understanding that the event handler will cause the Model to retain a reference to the ViewModel, and keep it from getting garbage collected.

1) Is this correct? How can I tell if these references are still present?

2) How should I determine the ViewModel is no longer needed and unsubscribe from the events?

Answer 1

I'm following on the answer of IAbstract, the WPF got this implemented directly via PropertyChangedEventManager, more can be found there.

The final code might look like:

public class DetailViewModel : IDisposable
{
    public DetailViewModel(MyDetailModel detailModel)
    {
        // Retain the Detail Model
        this.model = detailModel;

        // Handle changes to the Model not coming from this ViewModel
        if(model != null)
            PropertyChangedEventManager.AddHandler(model, model_PropertyChanged, "");
    }

    public void Dispose()
    {
        if(model != null)
            PropertyChangedEventManager.RemoveHandler(model, model_PropertyChanged, "");
    }
}

Answer 2

I'm a big fan of using IDisposable for this kind of thing. In fact, you can get excellent results using a CompositeDisposable to handle all of your clean-up needs.

Here's what I do:

public class DetailViewModel : IDisposable
{
    private readonly CompositeDisposable _disposables
        = new CompositeDisposable();

    public void Dispose()
    {
        _disposables.Dispose();
    }

    private readonly MyDetailModel _model;

    public DetailViewModel(MyDetailModel model)
    {
        _model = model;

        _model.PropertyChanged += _model_PropertyChanged;

        Action removeHandler = () =>
            _model.PropertyChanged -= _model_PropertyChanged;

        _disposables.Add(removeHandler);
    }

    private void _model_PropertyChanged(
        object sender, PropertyChangedEventArgs e)
    { /* ... */ }
}

What this lets you do is stick all sorts of clean-up code into a collection that automatically gets run once and only once when IDisposable.Dispose() gets called on your class.

This is particularly nice for event handlers as it allows you to place add handler code next to remove handler code in your source and this makes refactoring much simpler. It's very easy to see if you are actually removing handlers if the code is next to the add handler.

To make this happen you need to add two classes to your code.

The first is CompositeDisposable:

public sealed class CompositeDisposable : IEnumerable<IDisposable>, IDisposable
{
    private readonly List<IDisposable> _disposables;
    private bool _disposed;

    public CompositeDisposable()
    {
        _disposables = new List<IDisposable>();
    }

    public CompositeDisposable(IEnumerable<IDisposable> disposables)
    {
        if (disposables == null)
            { throw new ArgumentNullException("disposables"); }
        _disposables = new List<IDisposable>(disposables);
    }

    public CompositeDisposable(params IDisposable[] disposables)
    {
        if (disposables == null)
            { throw new ArgumentNullException("disposables"); }
        _disposables = new List<IDisposable>(disposables);
    }

    public void Add(IDisposable disposable)
    {
        if (disposable == null)
            { throw new ArgumentNullException("disposable"); }
        lock (_disposables)
        {
            if (_disposed)
            {
                disposable.Dispose();
            }
            else
            {
                _disposables.Add(disposable);
            }
        }
    }

    public IDisposable Add(Action action)
    {
        if (action == null) { throw new ArgumentNullException("action"); }
        var disposable = new AnonymousDisposable(action);
        this.Add(disposable);
        return disposable;
    }

    public IDisposable Add<TDelegate>(
            Action<TDelegate> add,
            Action<TDelegate> remove,
            TDelegate handler)
    {
        if (add == null) { throw new ArgumentNullException("add"); }
        if (remove == null) { throw new ArgumentNullException("remove"); }
        if (handler == null) { throw new ArgumentNullException("handler"); }
        add(handler);
        return this.Add(() => remove(handler));
    }

    public void Clear()
    {
        lock (_disposables)
        {
            var disposables = _disposables.ToArray();
            _disposables.Clear();
            Array.ForEach(disposables, d => d.Dispose());
        }
    }

    public void Dispose()
    {
        lock (_disposables)
        {
            if (!_disposed)
            {
                this.Clear();
            }
            _disposed = true;
        }
    }

    public IEnumerator<IDisposable> GetEnumerator()
    {
        lock (_disposables)
        {
            return _disposables.ToArray().AsEnumerable().GetEnumerator();
        }
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return this.GetEnumerator();
    }

    public bool IsDisposed
    {
        get
        {
            return _disposed;
        }
    }
}

And the second - which is used in CompositeDisposable - is AnonymousDisposable.

public sealed class AnonymousDisposable : IDisposable
{
    private readonly Action _action;
    private int _disposed;

    public AnonymousDisposable(Action action)
    {
        _action = action;
    }

    public void Dispose()
    {
        if (Interlocked.Exchange(ref _disposed, 1) == 0)
        {
            _action();
        }
    }
}

The class AnonymousDisposable is used to turn an Action into an IDisposable so that the action is run when the AnonymousDisposable is disposed.

A further option that you can now use easily is the use of anonymous event handlers rather than needing to define private methods to handle the events.

You could use this in the constructor instead:

        PropertyChangedEventHandler handler = (s, e) =>
        {
            // Use inline lambdas instead of private methods to handle events
        };

        model.PropertyChanged += handler;

        _disposables.Add(() => model.PropertyChanged -= handler);

You can use method-level variables in lamdbas, so this option can help keep your module getting all cluttered.

Now, you could stop at this, but you might have noticed another Add overload in the CompositeDisposable class that helps adding event subscriptions, like so:

        PropertyChangedEventHandler handler = (s, e) => { /* ... */ };

        _disposables.Add(
                    h => model.PropertyChanged += h,
                    h => model.PropertyChanged -= h,
                    handler);

This does the entire work of subscribing and unsubscribing from the handler.

You can even go one step further and do it all in one line, like this:

        _disposables.Add<PropertyChangedEventHandler>(
            h => model.PropertyChanged += h,
            h => model.PropertyChanged -= h,
            (s, e) =>
                {
                    // ...
                });

Sweet, huh?

I hope this helps.

Answer 3

At first I thought this would be the way to go:

public class DetailViewModel : IDisposable
{
    public DetailViewModel(MyDetailModel detailModel)
    {
        // Retain the Detail Model
        this.model = detailModel;

        // Handle changes to the Model not coming from this ViewModel
        this.model.PropertyChanged += model_PropertyChanged;  // Potential leak?
    }

    public void Dispose()
    {
        this.model.PropertyChanged -= model_PropertyChanged;
    }
}

But then I found this beautiful nugget. So, there are at least two possible solutions: (a) sample implementing IDisposable, and (b) arguments against IDisposable. I'll leave the debate to you. ;)

You may also consider the WeakEvent Pattern among others ...

Answer 4

You may want to consider using a Weak Event Pattern. I believe that Microsoft introduced WeakEventManager and IWeakEventListener to solve this exact garbage collection issue.