What is my mistake in implementing an asynchronous RelayCommand?

Question

I am learning WPF and MVVM at the moment and I faced a problem when i tried to write unit tests for a viewmodel, whose commands invoke async methods. That problem is well-described in this question. That question also has a solution: to write a new Command class with an additional awaitable method that can be awaited in unit tests. But since i use MvvmLight, i decided not to write a new class, but to inherit from the built-in RelayCommand class instead. However, i don't seem to understand how to do it properly. Below is a simplified example that illustrates my problem:

AsyncRelayCommand:

public class AsyncRelayCommand : RelayCommand
{
    private readonly Func<Task> _asyncExecute;

    public AsyncRelayCommand(Func<Task> asyncExecute)
        : base(() => asyncExecute())
    {
        _asyncExecute = asyncExecute;
    }

    public AsyncRelayCommand(Func<Task> asyncExecute, Action execute)
        : base(execute)
    {
        _asyncExecute = asyncExecute;
    }
    
    public Task ExecuteAsync()
    {
        return _asyncExecute();
    }

    //Overriding Execute like this fixes my problem, but the question remains unanswered.
    //public override void Execute(object parameter)
    //{
    //    _asyncExecute();
    //}
}

My ViewModel (based on the default MvvmLight MainViewModel):

public class MainViewModel : ViewModelBase
{
    private string _welcomeTitle = "Welcome!";

    public string WelcomeTitle
    {
        get
        {
            return _welcomeTitle;
        }

        set
        {
            _welcomeTitle = value;
            RaisePropertyChanged("WelcomeTitle");
        }
    }

    public AsyncRelayCommand Command { get; private set; }
    public MainViewModel(IDataService dataService)
    {
        Command = new AsyncRelayCommand(CommandExecute); //First variant
        Command = new AsyncRelayCommand(CommandExecute, () => CommandExecute()); //Second variant
    }

    private async Task CommandExecute()
    {
        WelcomeTitle = "Command in progress";
        await Task.Delay(1500);
        WelcomeTitle = "Command completed";
    }
}

As far as i understand it, both First and Second variants should invoke different constructors, but lead to the same result. However, only the second variant works the way i expect it to. The first one behaves strangely, for example, if i press the button, that is binded to Command once, it works ok, but if i try to press it a second time a few seconds later, it simply does nothing.

My understanding of async and await is far from complete. Please explain me why the two variants of instantiating the Command property behave so differently.

P.S.: this behavior is noticeable only when i inherit from RelayCommand. A newly created class that implements ICommand and has the same two constructors works as expected.

Answer 1

OK, I think I found the problem. RelayCommand uses a WeakAction to allow the owner (target) of the Action to be garbage collected. I'm not sure why they made this design decision.

So, in the working example where the () => CommandExecute() is in the view model constructor, the compiler is generating a private method on your constructor that looks like this:

[CompilerGenerated]
private void <.ctor>b__0()
{
  this.CommandExecute();
}

Which works fine because the view model is not eligible for garbage collection.

However, in the odd-behavior example where the () => asyncExecute() is in the constructor, the lambda closes over the asyncExecute variable, causing a separate type to be created for that closure:

[CompilerGenerated]
private sealed class <>c__DisplayClass2
{
  public Func<Task> asyncExecute;

  public void <.ctor>b__0()
  {
    this.asyncExecute();
  }
}

This time, the actual target of the Action is an instance of <>c__DisplayClass2, which is never saved anywhere. Since WeakAction only saves a weak reference, the instance of that type is eligible for garbage collection, and that's why it stops working.

If this analysis is correct, then you should always either pass a local method to RelayCommand (i.e., do not create lambda closures), or capture a (strong) reference to the resulting Action yourself:

private readonly Func<Task> _asyncExecute;
private readonly Action _execute;

public AsyncRelayCommand(Func<Task> asyncExecute)
    : this(asyncExecute, () => asyncExecute())
{
}

private AsyncRelayCommand(Func<Task> asyncExecute, Action execute)
    : base(execute)
{
  _asyncExecute = asyncExecute;
  _execute = execute;
}

Note that this actually has nothing to do with async; it's purely a question of lambda closures. I suspect it's the same underlying issue as this one regarding lambda closures with Messenger.