AsyncCommand using MVVM and WPF

Question

Goals

• I want to be able to call async code using the MVVM pattern
• I want to be able to add loading animations/screens without having to add properties for each command on my viewmodels
• I may want to be able to cancel these operations

This is what I came up with

public abstract class CommandBase : ICommand
{
    private readonly Func<bool> _canExecute;

    public CommandBase()
    {

    }

    public CommandBase(Func<bool> canExecute)
    {
        if (canExecute == null) throw new ArgumentNullException(nameof(canExecute));
        _canExecute = canExecute;
    }

    public bool CanExecute(object parameter)
    {
        return _canExecute == null || _canExecute();
    }

    public abstract void Execute(object parameter);

    public void RaiseCanExecuteChanged()
    {
        CanExecuteChanged?.Invoke(this, EventArgs.Empty);
    }

    public event EventHandler CanExecuteChanged;
}


public class AsyncCommand : CommandBase, INotifyPropertyChanged
{
    private readonly Func<CancellationToken, Task> _action;

    private CancellationTokenSource _cancellationTokenSource;

    private bool _isRunning;
    public bool IsRunning
    {
        get { return _isRunning; }
        set
        {
            _isRunning = value; 
            OnPropertyChanged();
        }
    }

    private ICommand _cancelCommand;
    public ICommand CancelCommand => _cancelCommand ?? (_cancelCommand = new RelayCommand(Cancel));

    public AsyncCommand(Func<CancellationToken, Task> action)
    {
        if (action == null) throw new ArgumentNullException(nameof(action));
        _action = action;
    }

    public AsyncCommand(Func<CancellationToken, Task> action, Func<bool> canExecute) : base(canExecute)
    {
        if (action == null) throw new ArgumentNullException(nameof(action));
        _action = action;
    }

    private void Cancel()
    {
        _cancellationTokenSource?.Cancel();
    }

    public override async void Execute(object parameter)
    {
        IsRunning = true;
        try
        {
            using (var tokenSource = new CancellationTokenSource())
            {
                _cancellationTokenSource = tokenSource;

                await ExecuteAsync(tokenSource.Token);
            }
        }
        finally
        {
            _cancellationTokenSource = null;
            IsRunning = false;
        }
    }

    private Task ExecuteAsync(CancellationToken cancellationToken)
    {
        return _action(cancellationToken);
    }

    public event PropertyChangedEventHandler PropertyChanged;

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
    {
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
    }
}

It can be used like this:

<Button Content="Start" Command="{Binding UpdateDisplayTextCommand}"/>
<Button Content="Cancel" Command="{Binding UpdateDisplayTextCommand.CancelCommand}"/>
<ProgressBar IsIndeterminate="{Binding UpdateDisplayTextCommand.IsRunning}" />

Any faults on this implementation? I'm also open to ideas on features which could be added to this class.

Answer 1

You should avoid code duplication in constructors. For larger objects this practice can quickly spin out of control. Re-use existing constructors instead:

public AsyncCommand(Func<CancellationToken, Task> action) : this(action, () => true) {}

or use default parameters:

public AsyncCommand(Func<CancellationToken, Task> action, Func<bool> canExecute = null) : base(canExecute)
{
    if (action == null) throw new ArgumentNullException(nameof(action));
    _action = action;
}

public CommandBase(Func<bool> canExecute = null)
{
    //this also allows you to remove "_canExecute == null" check
    _canExecute = canExecute ?? () => true;
}

Also, public constructors in abstract class look weird. You should make those protected.

Another minor thing: you have public and private members mixed together in a way, that does not make much sense. Personally, I find code easier to follow, if it has members of the same access level grouped together. But you can use any other scheme (some people like to group ICommand property with related delegate, for example), as long as it does not look random and chaotic.

P.S. If I were to use this class, I would also want to have an overloaded constructor, which takes Action<CancellationToken>. I don't want to manually create tasks, I want you to do it for me. I'm lazy like that. :) I would also want the command to either implement IDisposable or have a public Cancel method, so I can stop async operation programmatically. I mean I could call AsyncCommand.CancelCommand.Execute(...) but it looks ugly.

Answer 2

Looks well implemented.

Just one point: If IsRunning is true, CanExecute should return false to avoid multiple execution.

---

The AsyncCommand looks a little bit like a mixture of a command and a view model. I think it is ok for that case. However another option could be to create a view model "AsyncOperationViewModel" with the the properties Command, CancelCommand and IsRunning.

Answer 3

I realized that the question is quite old, but still seems to have some relevance. I guess people feel attracted, because they expect to find a usable asynchronous ICommand implementation.
The current implementation is full of flaws, of which most are not addressed by any answer.

The implementation needs serious fixes otherwise it is dangerous and a potential bug producer:

1. Why do you ignore the command parameter of Execute(object)?
2. Why are you not awaiting _action?
3. Why do you pass an internal CancellationToken to the async delegate, shouldn't the caller of your API be responsible to handle cancellation (like the whole Task library does)?
4. Do you know that when executing your command successively, you are overriding the reference to the previous CancellationTokenSource, which makes it impossible to cancel previous invocations?
5. Do you know that when executing your command successively, the first command handler that returns will set IsRunning to false, although there may still handlers executing?
6. Do you know that CancellationTokenSource implements IDisposable and must be disposed? Generally a class that references unmanaged resources and therefore implements IDisposable needs lifetime management to be disposed properly, otherwise your code introduces a potential memory leak or resource starvation?
7. The current implementation is not reusable (e.g. doesn't allow a command parameter).
8. The command can have a property like your IsRunning to return the internal busy state, but this property must have a private (or non-public) set method. For data binding, the client should define dedicated property, that can be set locally inside the actual command handler definition. It's common to return a new ICommand instance from a read-only computed property like:
   public ICommand SaveCommand => new AsyncRelayCommand(). In such a scenario the e.g. Button would have a different instance than e.g. the ProgressBar and therefore the IsRunning property would be a meaningless binding source for the ProgressBar. A good API must be flexible and should not force the developer to adapt a specific programming style. The bottom line: the client code must be responsible to track the execution of his command handler in order to get the most reliable/stable results.

The general idea of an asynchronous command is to allow to define asynchronous command handlers. This means the real asynchronous part is the responsibility of the client of the command API. The command itself is only the invocator of the command handler (like the common C# event pattern).
For this reason the client is responsible to decide whether his asynchronous operation is cancelable or not. He must provide the CancellationToken and is responsible to manage multiple instance of them (e.g., by linking them). The command API in general should also accept a CancellationToken following the fashion of the Task API or TAP library implements.

Another advantage of avoiding any CancellationTokenSource aggregation is the elimination of the responsibility to track the lifetime of a IDisposable or even having the asynchronous command itself to implement IDisposabe

Since async void signatures introduce some serious problems regarding error handling, the original ICommand API should be "hidden" by implementing the interface member ICommand.Execute explicitly. This way we can encourage the client to invoke the clean async Task custom overload e.g. async Task ExecuteAsync(object).

A clean and very basic non-generic solution, that also accepts synchronous command handlers (to support "default" synchronous command execution) could look as followed:

IAsyncRelayCommand.cs

public interface IAsyncRelayCommand : ICommand
{
  bool IsExecuting { get; }

  bool CanExecute();
  Task ExecuteAsync();
  Task ExecuteAsync(CancellationToken cancellationToken);
  Task ExecuteAsync(object parameter);
  Task ExecuteAsync(object parameter, CancellationToken cancellationToken);

  void InvalidateCommand();
}

AsyncRelayCommand.cs

public class AsyncRelayCommand : IAsyncRelayCommand
{
  public bool IsExecuting => this.executionCount > 0;

  protected readonly Func<Task> ExecuteAsyncNoParam;
  protected readonly Action ExecuteNoParam;
  protected readonly Func<bool> CanExecuteNoParam;

  private readonly Func<object, Task> executeAsync;
  private readonly Action<object> execute;
  private readonly Predicate<object> canExecute;
  private EventHandler canExecuteChangedDelegate;
  private int executionCount;

  public event EventHandler CanExecuteChanged
  {
    add
    {
      CommandManager.RequerySuggested += value;
      this.canExecuteChangedDelegate = (EventHandler) Delegate.Combine(this.canExecuteChangedDelegate, value);
    }
    remove
    {
      CommandManager.RequerySuggested -= value;
      this.canExecuteChangedDelegate = (EventHandler) Delegate.Remove(this.canExecuteChangedDelegate, value);
    }
  }

  #region Constructors

  public AsyncRelayCommand(Action<object> execute)
    : this(execute, param => true)
  {
  }

  public AsyncRelayCommand(Action executeNoParam)
    : this(executeNoParam, () => true)
  {
  }

  public AsyncRelayCommand(Func<object, Task> executeAsync)
    : this(executeAsync, param => true)
  {
  }

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

  public AsyncRelayCommand(Action executeNoParam, Func<bool> canExecuteNoParam)
  {
    this.ExecuteNoParam = executeNoParam ?? throw new ArgumentNullException(nameof(executeNoParam));
    this.CanExecuteNoParam = canExecuteNoParam ?? (() => true);
  }

  public AsyncRelayCommand(Action<object> execute, Predicate<object> canExecute)
  {
    this.execute = execute ?? throw new ArgumentNullException(nameof(execute));
    this.canExecute = canExecute ?? (param => true); ;
  }

  public AsyncRelayCommand(Func<Task> executeAsyncNoParam, Func<bool> canExecuteNoParam)
  {
    this.ExecuteAsyncNoParam = executeAsyncNoParam ?? throw new ArgumentNullException(nameof(executeAsyncNoParam));
    this.CanExecuteNoParam = canExecuteNoParam ?? (() => true);
  }

  public AsyncRelayCommand(Func<object, Task> executeAsync, Predicate<object> canExecute)
  {
    this.executeAsync = executeAsync ?? throw new ArgumentNullException(nameof(executeAsync));
    this.canExecute = canExecute ?? (param => true); ;
  }

  #endregion Constructors

  
  public bool CanExecute() => CanExecute(null);

  public bool CanExecute(object parameter) => this.canExecute?.Invoke(parameter)
                                              ?? this.CanExecuteNoParam?.Invoke()
                                              ?? true;

  async void ICommand.Execute(object parameter) => await ExecuteAsync(parameter, CancellationToken.None);

  public async Task ExecuteAsync() => await ExecuteAsync(null, CancellationToken.None);

  public async Task ExecuteAsync(CancellationToken cancellationToken) => await ExecuteAsync(null, cancellationToken);

  public async Task ExecuteAsync(object parameter) => await ExecuteAsync(parameter, CancellationToken.None);
 
  public async Task ExecuteAsync(object parameter, CancellationToken cancellationToken)
  {
    try
    {
      Interlocked.Increment(ref this.executionCount);
      cancellationToken.ThrowIfCancellationRequested();

      if (this.executeAsync != null)
      {
        await this.executeAsync.Invoke(parameter).ConfigureAwait(false);
        return;
      }
      if (this.ExecuteAsyncNoParam != null)
      {
        await this.ExecuteAsyncNoParam.Invoke().ConfigureAwait(false);
        return;
      }
      if (this.ExecuteNoParam != null)
      {
        this.ExecuteNoParam.Invoke();
        return;
      }

      this.execute?.Invoke(parameter);
    }
    finally
    {
      Interlocked.Decrement(ref this.executionCount);
    }
  }

  public void InvalidateCommand() => OnCanExecuteChanged();

  protected virtual void OnCanExecuteChanged() => this.CanExecuteChangedDelegate?.Invoke(this, EventArgs.Empty);
}