How do I remove event handlers when I'm finished with a View and ViewModel, but not the Model

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?

Solution 1

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 ...

Solution 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.

Solution 3

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.

Comments

[mbmcavoy]

It's taking a little while for me to digest this - my brain asplode! I still don't understand what calls Dispose(). Do I need to determine when I am done with the ViewModel, and call it myself?

[mbmcavoy]

Good reference. I really appreciate that @LBugnion himself answered that question, as I am using MVVM-Light myself.

[Enigmativity]

@mbmcavoy - You need to call Dispose. What my solution does is allows you to build up the set of clean-up actions throughout your code and have a single easy way to clean it all up.

[mbmcavoy]

The simple IDisposable implementation does the job!