How to store Actions that do not prevent garbage collection of variables they use in scope

Question

I'm trying to fix a garbage collection problem of a MVVM application which uses the following model of Undo stack.

The example is very minimalistic and real world code is much different, uses a factory class of undo lists per ViewModel instead of a single undolist but is representative:

using System;
using System.Collections.Generic;
using System.Text;
using System.Diagnostics;
using System.Reflection;
using System.ComponentModel;
using System.Linq;

namespace ConsoleApplication9
{
    public class UndoList
    {
        public bool IsUndoing { get; set; }

        private Stack<Action> _undo = new Stack<Action>();

        public Stack<Action> Undo
        {
            get { return _undo; }
            set { _undo = value; }
        }

        private static UndoList _instance;
        // singleton of the undo stack
        public static UndoList Instance
        {
            get
            {
                if (_instance == null)
                {
                    _instance = new UndoList();
                }
                return _instance;
            }
        }
    }

    public class ViewModel : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;

        // execute the last undo operation
        public void Undo()
        {
            UndoList.Instance.IsUndoing = true;
            var action = UndoList.Instance.Undo.Pop();
            action();
            UndoList.Instance.IsUndoing = false;
        }

        // push an action into the undo stack
        public void AddUndo(Action action)
        {
            if (UndoList.Instance.IsUndoing) return;

            UndoList.Instance.Undo.Push(action);
        }

        // create push an action into the undo stack that resets a property value
        public void AddUndo(string propertyName, object oldValue)
        {
            if (UndoList.Instance.IsUndoing) return;

            var property = this.GetType().GetProperties().First(p => p.Name == propertyName);

            Action action = () =>
            {
                property.SetValue(this, oldValue, null);
            };

            UndoList.Instance.Undo.Push(action);
        }
    }

    public class TestModel : ViewModel
    {
        private bool _testProperty;
        public bool TestProperty
        {
            get
            {
                return _testProperty;
            }
            set
            {
                base.AddUndo("TestProperty", _testProperty);
                _testProperty = value;
            }
        }

        // mock property indicating if a business action has been done for test
        private bool _hasBusinessActionBeenDone;
        public bool HasBusinessActionBeenDone
        {
            get
            {
                return _hasBusinessActionBeenDone;
            }
            set
            {
                _hasBusinessActionBeenDone = value;
            }
        }

        public void DoBusinessAction()
        {
            AddUndo(() => { inverseBusinessAction(); });
            businessAction();
        }

        private void businessAction()
        {
            // using fake property for brevity of example
            this.HasBusinessActionBeenDone = true;
        }

        private void inverseBusinessAction()
        {
            // using fake property for brevity of example
            this.HasBusinessActionBeenDone = false;
        }

    }

    class Program
    {
        static void Test()
        {
            var vm = new TestModel();

            // test undo of property
            vm.TestProperty = true;
            vm.Undo();
            Debug.Assert(vm.TestProperty == false);

            // test undo of business action
            vm.DoBusinessAction();
            vm.Undo();
            Debug.Assert(vm.HasBusinessActionBeenDone == false);

            // do it once more without Undo, so the undo stack has something
            vm.DoBusinessAction();
        }

        static void Main(string[] args)
        {
            Program.Test();
            GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);

            // at this point UndoList.Instance.Undo
            // contains an Action which references the TestModel
            // which will never be collected...
            // in real world code knowing when to clear this is a problem
            // because it is a singleton factory class for undolists per viewmodel type
            // ideally would be to clear the list when there are no more references
            // to the viewmodel type in question, but the Actions in the list prevent that
        }
    }
}

You see that when any viewModel goes out of scope the actions in the UndoList keep references to them. The real code groups various viewmodels into grouped undolists (viewModels that contain child viewmodels share the same undo stack), so it is difficult to know when and where to put the clearing.

I was wondering if there is some method to make those actions expire if they are the only one keeping references to the variables inside them?

Suggestions welcome!

Answer 1

I've got a solution for you. I don't like the use of the UndoList as a singleton, but I've kept it to provide you with a direct answer to your question. In practice I wouldn't use a singleton.

Now, you will find it very difficult to avoid capturing references to your view models in your actions. It would make your code very ugly if you tried. The best approach is to make your view models implement IDisposable and make sure that you dispose of them when they go out of scope. Remember that the garbage collector never calls Dispose so you must.

Using IDisposable is the standard model for cleaning up when an instance is no longer needed.

So the first thing to define is a helper class that executes an action when it is disposed.

public sealed class AnonymousDisposable : IDisposable
{
    private readonly Action _dispose;
    private int _isDisposed;

    public AnonymousDisposable(Action dispose)
    {
        _dispose = dispose;
    }

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

Now I can write things like this to remove elements from lists:

var disposable = new AnonymousDisposable(() => list.Remove(item));

Later, when I call disposable.Dispose() the item is removed from the list.

Now here's your code re-implemented.

I've changed UndoList to be a static class, not a singleton. You can change it back if need be.

public static class UndoList
{
    public static bool IsUndoing { get; private set; }
    private static List<Action> _undos = new List<Action>();

    public static IDisposable AddUndo(Action action)
    {
        var disposable = (IDisposable)null;
        if (!IsUndoing)
        {           
            disposable = new AnonymousDisposable(() => _undos.Remove(action));
            _undos.Add(action);
        }
        return disposable ?? new AnonymousDisposable(() => { });
    }

    public static bool Undo()
    {
        IsUndoing = true;
        var result = _undos.Count > 0;
        if (result)
        {
            var action = _undos[_undos.Count - 1];
            _undos.Remove(action);
            action();
        }
        IsUndoing = false;
        return result;
    }
}

You'll notice that I've replaced the stack with a list. I did that because I need to remove items from inside the list.

Also, you can see that AddUndo now returns an IDisposable. Calling code needs to keep the return disposable and call Dispose when it wants to remove the action from the list.

I've also internalized the Undo action. It didn't make sense to have it in the view model. Calling Undo effectively pops the top item off of the list and executes the action and returns true. However, if the list is empty it returns false. You can use this for testing purposes.

The ViewModel class now looks like this:

public class ViewModel : IDisposable, INotifyPropertyChanged
{
    public ViewModel()
    {
        _disposables = new List<IDisposable>();
        _disposable = new AnonymousDisposable(() =>
            _disposables.ForEach(d => d.Dispose()));
    }

    private readonly List<IDisposable> _disposables;
    private readonly IDisposable _disposable;

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

    public event PropertyChangedEventHandler PropertyChanged;

    protected void AddUndo(Action action)
    { ... }

    protected void SetUndoableValue<T>(Action<T> action, T newValue, T oldValue)
    { ... }
}

It implements IDisposable and internally, keeps track of a list of disposables and an anonymous disposable that will dispose of the items in the list when the view model itself is disposed of. Whew! A mouthful, but I hope that makes sense.

The AddUndo method body is this:

    protected void AddUndo(Action action)
    {
        var disposable = (IDisposable)null;
        Action inner = () =>
        {
            _disposables.Remove(disposable);
            action();
        };
        disposable = UndoList.AddUndo(inner);
        _disposables.Add(disposable);
    }

Internally it calls UndoList.AddUndo passing in an action that will remove the returned IDisposable from the view model's list of undo actions when UndoList.Undo() is called - as well as, importantly, actually executing the action.

So this means that when the view model is disposed all of its outstanding undo actions are removed from the undo list and when Undo is called the associated disposable is removed from the view model. And this ensures that you are not keeping references to the view model when it is disposed of.

I created a helper function called SetUndoableValue that replaced your void AddUndo(string propertyName, object oldValue) method which wasn't strongly-typed and could cause you to have run-time errors.

    protected void SetUndoableValue<T>(Action<T> action, T newValue, T oldValue)
    {
        this.AddUndo(() => action(oldValue));
        action(newValue);
    }

I made both of these methods protected as public seemed too promiscuous.

The TestModel is more-or-less the same:

public class TestModel : ViewModel
{
    private bool _testProperty;
    public bool TestProperty
    {
        get { return _testProperty; }
        set
        {
            this.SetUndoableValue(v => _testProperty = v, value, _testProperty);
        }
    }

    public bool HasBusinessActionBeenDone { get; set; }

    public void DoBusinessAction()
    {
        this.AddUndo(this.inverseBusinessAction);
        businessAction();
    }

    private void businessAction()
    {
        this.HasBusinessActionBeenDone = true;
    }

    private void inverseBusinessAction()
    {
        this.HasBusinessActionBeenDone = false;
    }
}

And finally, here's the code that tests the UndoList functions correctly:

using (var vm = new TestModel())
{
    Debug.Assert(UndoList.Undo() == false);

    vm.TestProperty = true;

    Debug.Assert(UndoList.Undo() == true);
    Debug.Assert(UndoList.Undo() == false);

    Debug.Assert(vm.TestProperty == false);

    vm.DoBusinessAction();

    Debug.Assert(UndoList.Undo() == true);

    Debug.Assert(vm.HasBusinessActionBeenDone == false);

    vm.DoBusinessAction();
}

Debug.Assert(UndoList.Undo() == false);

Please let me know if I can provide any more detail on anything.

Answer 2

If you can't clean it up any other way you could use WeakReference to hold property, but I think there would be other issues because this would still cause a Action instance to exist with a null reference attached to it.

As a quick look I would be more inclined to use the singleton to hold a registration to the model and let the model manage a instance list of all the undo actions attached to it. When the model goes out of scope call a clean-up method on it or implement a IDisposable type interface on it may if this fits. However depending on the implementation you may not need the singleton anyway.