RelayCommand Memory leak

Question

I am looking for an implementation of RelayCommand. The original implementation that I considered was the classic one (lets call it implementation A)

public class RelayCommand : ICommand
{        
    private readonly Predicate<object> canExecute;

    private readonly Action<object> execute;

    private EventHandler canExecuteEventhandler;

    public RelayCommand(Action<object> execute)
        : this(execute, null)
    {
    }

    public RelayCommand(Action<object> execute, Predicate<object> canExecute)
    {
        if (execute == null)
        {
            throw new ArgumentNullException("execute");
        }

        this.execute = execute;
        this.canExecute = canExecute;
    }

    public event EventHandler CanExecuteChanged
    {
        add
        {
            this.canExecuteEventhandler += value;
        }

        remove
        {
            this.canExecuteEventhandler -= value;
        }
    }

    [DebuggerStepThrough]
    public bool CanExecute(object parameter)
    {
        return this.canExecute == null ? true : this.canExecute(parameter);
    }

    [DebuggerStepThrough]
    public void Execute(object parameter)
    {
        this.execute(parameter);
    }

    public void InvokeCanExecuteChanged()
    {
        if (this.canExecute != null)
        {
            if (this.canExecuteEventhandler != null)
            {
                this.canExecuteEventhandler(this, EventArgs.Empty);
            }
        }
    }
}

This is the implementation that I have used since I started developing in Silverlight around 2009. I have also used it in WPF applications. Lately I understood that it has a memory leak problem in cases where the views that bind to the command have shorter life span than the command itself. Apparently when a button binds to the command, it of course registers to the CanExecuteChanged event handler but never unregistered. The default event handlers hold strong reference to the delegate, which holds a strong reference to the button itself, therefore the RelayCommand keeps the button alive and that's a memory leak.

Another implementation that I have found uses the CommandManager. The CommandManager exposes a a RequerySuggested event and internally only hold weak references to the delegates. So the definition of the event can be implemented as follows (implementation B)

public event EventHandler CanExecuteChanged
{
    add { CommandManager.RequerySuggested += value; }
    remove { CommandManager.RequerySuggested -= value; }
}

public void RaiseCanExecuteChanged()
{
    CommandManager.InvalidateRequerySuggested();
}

So that every delegate is passed to the static event handler instead of being held by the relay command itself. My problem with this implementation is that it relies on the CommandManager to know when to raise the event. Also, when RaiseCanExecuteChanged is called, the command manager raises this event for all RelayCommands, not specifically the one that initiated the event.

The last implementation I found was from MvvmLight where the event is defined as such (implementation C):

public event EventHandler CanExecuteChanged
{
    add
    {
        if (_canExecute != null)
        {
            // add event handler to local handler backing field in a thread safe manner
            EventHandler handler2;
            EventHandler canExecuteChanged = _requerySuggestedLocal;

            do
            {
                handler2 = canExecuteChanged;
                EventHandler handler3 = (EventHandler)Delegate.Combine(handler2, value);
                canExecuteChanged = System.Threading.Interlocked.CompareExchange<EventHandler>(
                    ref _requerySuggestedLocal, 
                    handler3, 
                    handler2);
            } 
            while (canExecuteChanged != handler2); 

            CommandManager.RequerySuggested += value;
        }
    }

    remove
    {
        if (_canExecute != null)
        {
            // removes an event handler from local backing field in a thread safe manner
            EventHandler handler2;
            EventHandler canExecuteChanged = this._requerySuggestedLocal;

            do
            {
                handler2 = canExecuteChanged;
                EventHandler handler3 = (EventHandler)Delegate.Remove(handler2, value);
                canExecuteChanged = System.Threading.Interlocked.CompareExchange<EventHandler>(
                    ref this._requerySuggestedLocal, 
                    handler3, 
                    handler2);
            } 
            while (canExecuteChanged != handler2); 

            CommandManager.RequerySuggested -= value;
        }
    }
}

So in addition to the command manager it also holds the delegate locally and does some magic trick to support thread safety.

My questions are:

1. Which of these implementations actually solve the memory leak problem.
2. Is there an implementation that solves the problem without relying on the CommandManager?
3. Is the trick that is done in implementation C really necessary to avoid thread safety related bugs and how does it solve it?

Answer 1

You can use the WeakEventManager.

public event EventHandler CanExecuteChanged
{
    add
    {
        RelayCommandWeakEventManager.AddHandler(this, value);
    }

    remove
    {
        RelayCommandWeakEventManager.RemoveHandler(this, value);
    }
}

private class RelayCommandWeakEventManager : WeakEventManager
{
    private RelayCommandWeakEventManager()
    {
    }
    public static void AddHandler(RelayCommand source, EventHandler handler)
    {
        if (source == null)
            throw new ArgumentNullException("source");
        if (handler == null)
            throw new ArgumentNullException("handler");

        CurrentManager.ProtectedAddHandler(source, handler);
    }
    public static void RemoveHandler(RelayCommand source, 
                                 EventHandler handler)
    {
        if (source == null)
            throw new ArgumentNullException("source");
        if (handler == null)
            throw new ArgumentNullException("handler");

        CurrentManager.ProtectedRemoveHandler(source, handler);
    }

    private static RelayCommandWeakEventManager CurrentManager
    {
        get
        {
            Type managerType = typeof(RelayCommandWeakEventManager);
            RelayCommandWeakEventManager manager = 
                (RelayCommandWeakEventManager)GetCurrentManager(managerType);

            // at first use, create and register a new manager
            if (manager == null)
            {
                manager = new RelayCommandWeakEventManager();
                SetCurrentManager(managerType, manager);
            }

            return manager;
        }
    }


    /// <summary>
    /// Return a new list to hold listeners to the event.
    /// </summary>
    protected override ListenerList NewListenerList()
    {
        return new ListenerList<EventArgs>();
    }


    /// <summary>
    /// Listen to the given source for the event.
    /// </summary>
    protected override void StartListening(object source)
    {
        EventSource typedSource = (RelayCommand) source;
        typedSource.canExecuteEventhandler += new EventHandler(OnSomeEvent);
    }

    /// <summary>
    /// Stop listening to the given source for the event.
    /// </summary>
    protected override void StopListening(object source)
    {
        EventSource typedSource = (RelayCommand) source;
        typedSource.canExecuteEventhandler -= new EventHandler(OnSomeEvent);
    }

    /// <summary>
    /// Event handler for the SomeEvent event.
    /// </summary>
    void OnSomeEvent(object sender, EventArgs e)
    {
        DeliverEvent(sender, e);
    }
}

This code was shamelessly lifted (and adapted) from https://msdn.microsoft.com/en-us/library/aa970850%28v=vs.110%29.aspx

Answer 2

Based on Aron's answer, I went with a solution that involves weak events, but developed differently, in order to reduce the amounts of code and make the building blocks a little more reusable.

The following implementation mixes the "classic" one, with some ideas taken from MvvmLight and I am using a WeakEvent class that is developed according to a pattern introduced in the following (excellent!!!) article by Daniel Grunwald. http://www.codeproject.com/Articles/29922/Weak-Events-in-C

The RelayCommand itself is implemented as follows:

public class RelayCommand : ICommand
{
    private readonly Action _execute;
    private readonly Func<bool> _canExecute;
    private WeakEvent<EventHandler> _canExecuteChanged;

    /// <summary>
    /// Initializes a new instance of the RelayCommand class that 
    /// can always execute.
    /// </summary>
    /// <param name="execute">The execution logic.</param>
    /// <exception cref="ArgumentNullException">If the execute argument is null.</exception>
    public RelayCommand(Action execute)
        : this(execute, null)
    {
    }

    /// <summary>
    /// Initializes a new instance of the RelayCommand class.
    /// </summary>
    /// <param name="execute">The execution logic.</param>
    /// <param name="canExecute">The execution status logic.</param>
    /// <exception cref="ArgumentNullException">If the execute argument is null.</exception>
    public RelayCommand(Action execute, Func<bool> canExecute)
    {
        if (execute == null)
        {
            throw new ArgumentNullException("execute");
        }

        _execute = execute;
        _canExecute = canExecute;
        _canExecuteChanged = new WeakEvent<EventHandler>();
    }

    /// <summary>
    /// Occurs when changes occur that affect whether the command should execute.
    /// </summary>
    public event EventHandler CanExecuteChanged
    {
        add
        {
            _canExecuteChanged.Add(value);
        }

        remove
        {
            _canExecuteChanged.Remove(value);
        }
    }

    /// <summary>
    /// Raises the <see cref="CanExecuteChanged" /> event.
    /// </summary>
    [SuppressMessage(
        "Microsoft.Performance", 
        "CA1822:MarkMembersAsStatic",
        Justification = "The this keyword is used in the Silverlight version")]
    [SuppressMessage(
        "Microsoft.Design", 
        "CA1030:UseEventsWhereAppropriate",
        Justification = "This cannot be an event")]
    public void RaiseCanExecuteChanged()
    {
        _canExecuteChanged.Raise(this, EventArgs.Empty);
    }

    /// <summary>
    /// Defines the method that determines whether the command can execute in its current state.
    /// </summary>
    /// <param name="parameter">This parameter will always be ignored.</param>
    /// <returns>true if this command can be executed; otherwise, false.</returns>
    public bool CanExecute(object parameter)
    {
        return (_canExecute == null) || (_canExecute());
    }

    /// <summary>
    /// Defines the method to be called when the command is invoked. 
    /// </summary>
    /// <param name="parameter">This parameter will always be ignored.</param>
    public virtual void Execute(object parameter)
    {
        if (CanExecute(parameter)) 
        {
            _execute();
        }
    }
}

Note that I am not holding a weak refernces to the _execute and _canExecute delegates. Using weak references to delegates causes all sorts of problems when the delegates are closures since their target object is not referenced by any object and they "die" instantly. I expect these delegates to have the owner of the RelayCommand anyway, so their lifespan is expected to be the same as the RelayCommand's.

The CanExecuteChanged event is implemented using the WeakEvent, so even if the listeners do not unregister, the relay command does not affect their lifespan.