Is Josh Smith's implementation of the RelayCommand flawed?

Question

Consider the reference Josh Smith' article WPF Apps With The Model-View-ViewModel Design Pattern, specifically the example implementation of a RelayCommand (In Figure 3). (No need to read through the entire article for this question.)

In general, I think the implementation is excellent, but I have a question about the delegation of CanExecuteChanged subscriptions to the CommandManager's RequerySuggested event. The documentation for RequerySuggested states:

Since this event is static, it will only hold onto the handler as a weak reference. Objects that listen for this event should keep a strong reference to their event handler to avoid it being garbage collected. This can be accomplished by having a private field and assigning the handler as the value before or after attaching to this event.

Yet the sample implementation of RelayCommand does not maintain any such to the subscribed handler:

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

1. Does this leak the weak reference up to the RelayCommand's client, requiring that the user of the RelayCommand understand the implementation of CanExecuteChanged and maintain a live reference themselves?

2. If so, does it make sense to, e.g., modify the implementation of RelayCommand to be something like the following to mitigate the potential premature GC of the CanExecuteChanged subscriber:

   // This event never actually fires.  It's purely lifetime mgm't.
   private event EventHandler canExecChangedRef;
   public event EventHandler CanExecuteChanged
   {
       add 
       { 
           CommandManager.RequerySuggested += value;
           this.canExecChangedRef += value;
       }
       remove 
       {
           this.canExecChangedRef -= value;
           CommandManager.RequerySuggested -= value; 
       }
   }
   

Kellogg

Great question... I also wonder htf is RequerySuggested knowing when is a requery necessary.. – Upolu

RequerySuggested is documented to fire when the CanExecute state may need to be refreshed. I believe it can also be manually fired by clients if the client knows something that WPF itself doesn't know. – Kellogg

@Greg - And by what parameters does RequerySuggested know if CanExecute state may need to be refreshed? :) – Mump

@VitalyB: It knows that the state may need to be refreshed if RequerySuggested is fired at all. What do you mean "by what parameters"? – Kellogg

@Greg: You've written "RequerySuggested is documented to fire when the CanExecute state may need to be refreshed". How does it know when CatnExecute may need to be refreshed? How it evaluates the need? – Mump

That's up to the framework, I don't know if it's documented anywhere b/c it might change. If you write code that requires a particular implementation, you're writing bad code. If you need it to fire in a particular situation, you can trigger it yourself via CommandManager.RequerySuggested. – Kellogg

Answer 1

I too believe this implementation is flawed, because it definitely leaks the weak reference to the event handler. This is something actually very bad.
I am using the MVVM Light toolkit and the RelayCommand implemented therein and it is implemented just as in the article.
The following code will never invoke OnCanExecuteEditChanged:

private static void OnCommandEditChanged(DependencyObject d, 
                                         DependencyPropertyChangedEventArgs e)
{
    var @this = d as MyViewBase;
    if (@this == null)
    {
        return;
    }

    var oldCommand = e.OldValue as ICommand;
    if (oldCommand != null)
    {
        oldCommand.CanExecuteChanged -= @this.OnCanExecuteEditChanged;
    }
    var newCommand = e.NewValue as ICommand;
    if (newCommand != null)
    {
        newCommand.CanExecuteChanged += @this.OnCanExecuteEditChanged;
    }
}

However, if I change it like this, it will work:

private static EventHandler _eventHandler;

private static void OnCommandEditChanged(DependencyObject d,
                                         DependencyPropertyChangedEventArgs e)
{
    var @this = d as MyViewBase;
    if (@this == null)
    {
        return;
    }
    if (_eventHandler == null)
        _eventHandler = new EventHandler(@this.OnCanExecuteEditChanged);

    var oldCommand = e.OldValue as ICommand;
    if (oldCommand != null)
    {
        oldCommand.CanExecuteChanged -= _eventHandler;
    }
    var newCommand = e.NewValue as ICommand;
    if (newCommand != null)
    {
        newCommand.CanExecuteChanged += _eventHandler;
    }
}

The only difference? Just as indicated in the documentation of CommandManager.RequerySuggested I am saving the event handler in a field.

Tamikatamiko

Answer 2

I've found the answer in Josh's comment on his "Understanding Routed Commands" article:

[...] you have to use the WeakEvent pattern in your CanExecuteChanged event. This is because visual elements will hook that event, and since the command object might never be garbage collected until the app shuts down, there is a very real potential for a memory leak. [...]

The argument seems to be that CanExecuteChanged implementors must only hold weakly to the registered handlers, since WPF Visuals are to stupid to unhook themselves. This is most easily implemented by delegating to the CommandManager, who already does this. Presumably for the same reason.

Tetradymite

It has been nearly 10 years since you posted this answer. I wonder if this holds true now with .NET 4.8 (or 5.0) – Empire

Sadly, I haven't touched C# in the last 6 years, so I have no idea. – Tetradymite

Answer 3

Well, according to Reflector it's implemented the same way in the RoutedCommand class, so I guess it must be OK... unless someone in the WPF team made a mistake ;)

Terminal

I admit that it's unlikely, especially given that WPF is, I suspect, the common consumer of the CanExecuteChanged event. I'm trying to follow the docs as-written, though, so while the actual behavior may work, I can't help but suspect we're depending on an implementation detail. – Kellogg

Answer 4

I believe it is flawed.

By rerouting the events to the CommandManager, you do get the following behavior

This ensures that the WPF commanding infrastructure asks all RelayCommand objects if they can execute whenever it asks the built-in commands.

However, what happens when you wish to inform all controls bound to a single command to re-evaluate CanExecute status? In his implementation, you must go to the CommandManager, meaning

Every single command binding in your application is reevaluated

That includes all the ones that don't matter a hill of beans, the ones where evaluating CanExecute has side effects (such as database access or long running tasks), the ones that are waiting to be collected... Its like using a sledgehammer to drive a friggen nail.

You have to seriously consider the ramifications of doing this.

Shirt

In principle I agree with you. But on the other hand, CanExecute shouldn't really do anything heavy anyway since it can be called quite frequently by WPF – Mordacious

@Isak yep. Of course, that's not always reality. And in those cases blindly redirecting the event is a bad choice. – Shirt

Answer 5

I may be missing the point here but doesn't the following constitute the strong reference to the event handler in the contructor?

    _canExecute = canExecute;           

Secundine

CanExecuteChanged is not canExecute. – Kellogg

No. _canExecute is the delegate that evaluates whether the command can be executed. CanExecuteChanged is an event that occurs when _canExecute is reevaluated. There's no relation between _canExecute and the handlers subscribed to the CanExecuteChanged event – Terminal

Of course. My answer was pre-today's Red Bull equivalent! Is RouteCommand actually the object listening for the event? My understanding here is limited but what I'm seeing is an object that's going to create a listener for the CanExecuteChanged event and then in the example given it'll execute _saveCommand.CanExecuteChanged += myHandler. To my (very) caffeine addled brain, it would seem that the responsibility for the strong reference lies with SaveCommand, not RelayCommand as RelayCommand isn't providing the listener for CanExecuteChanged. – Secundine