Ability to customize exception flow in AsyncRelayCommand types

[Sergio0694]

Overview

This issue is about discussing ways to improve how AsyncRelayCommand types handle exceptions. Specifically, as discussed with @brminnick and as people have brought up in the past as well (eg. #22, #175, #251, etc.), the way async relay commands currently work is that if you invoke them through Execute (ie. through a synchronous API), the Task object is not awaited. This is by design, as it allows exceptions to be observable through the ExecutionTask property afterwards, or to otherwise bubble up to TaskScheduler.UnobservedTaskException to enable centralized logging. Currently, if someone wants to await a task, ExecuteAsync needs to be called instead. This also has the characteristic that calling Execute will not crash the app if the command fails.

Now, there are cases where all this is desireable, but there might also be cases where developers would want the exception to just be reported immediately and crash the app, for instance to more easily spot errors and to avoid the app getting into an inconsistent state, in case an exception was not planned to be raised from the wrapped methods at all.

Because of this, we're thinking about ways we could expose this additional flexibility for consumers.

This issue is meant to be a place to discuss this and the various API proposals related to this.

cc. @michael-hawker @Arlodotexe

API breakdown

One possible way to go could be to introduce something like this:

namespace CommunityToolkit.Mvvm.Input;

[Flags]
public enum AsyncRelayCommandOptions
{
    Default,
    AllowConcurrentExecution,
    FlowExceptionsToThreadPool,
    FlowExceptionsToSynchronizationContext,
    // Potentially more options in the future
}

This would then be used both in constructors for AsyncRelayCommand, as well as [ICommand].

Worth noting, this would replace the current bool allowConcurrentExecutions parameters in the constructors, as instead we would just take an AsyncRelayCommandOptions options parameter to specify all options. This way, consumers would have a single way to customize the behavior of the async command on all supported aspects.

As far as the source generator approach goes, we could do something like:

// Before
[ICommand(AllowConcurrentExecutions = true)]
private async Task DoSomethingAsync()
{
    // ...
}

// After
[ICommand(Options = AsyncRelayCommandOptions.AllowConcurrentExecutions)]
private async Task DoSomethingAsync()
{
    // ...
}

This is a bit more verbose than before. It might be ok, but I'm also open to suggestions.

Overall, using something like this would give us the following benefits:

• Everyone would be happy, as we could support all scenarios we wanted
• Having some options would give us room to potentially add more flags in the future without breaking or needing new APIs

Open questions

Should the default be the same behavior as today, or should the default be to rethrow in some specific way?

Alternatives

There isn't really a good alternative to have the app crash today. I mean you could do that but it'd be incredibly verbose, as you'd need to register a handler for PropertyChanged on the command, check whether the task changed, get it, check if it faulted, then post to the synchronization context you need and from there re-throw the exceptions. Not really ideal.

[powerdude]

I think there's value in allowing something like this:

[ICommand(Options = AsyncRelayCommandOptions.AllowConcurrentExecutions, ErrorHandler = nameof(HandleException))]
private async Task DoSomethingAsync()
{
    // ...
}


private void HandleException( Exception e)
{
////
}

or this:

/// this method is called from a try..catch handler that can surround the call to DoSomethingAsync 
partial void HandleError( Exception e)
{

}

[ICommand(Options = AsyncRelayCommandOptions.AllowConcurrentExecution)]
private async Task DoSomethingAsync()
{
    // ...
}

or this:

// defined by MVVM library
public interface IExceptionHandler
{
     void Handle( Exception e);
}

public class MyHandler:IExceptionHandler
{
   public void Handle( Exception e)
   {
     // do something cool
   }
}

[ICommand(Options = AsyncRelayCommandOptions.AllowConcurrentExecutions, ErrorHandler = typeof(MyHandler))]
private async Task DoSomethingAsync()
{
    // ...
}

This is something in the same vein of the ReactiveUI pattern with commands:

// Here we declare a ReactiveCommand, an OAPH and a property.
private readonly ObservableAsPropertyHelper<List<User>> _users;
public ReactiveCommand<Unit, List<User>> LoadUsers { get; }

// Create a command with asynchronous execution logic. The 
// command is always available for execution, it triggers
// LoadUsersAsync method which returns Task<List<User>>.
LoadUsers = ReactiveCommand.CreateFromTask(LoadUsersAsync);

// Here we subscribe to all exceptions thrown by our 
// command and log them using ReactiveUI logging system.
// If we forget to do this, our application will crush
// if anything goes wrong in LoadUsers command.
LoadUsers.ThrownExceptions.Subscribe(exception => 
{
    this.Log().Warn("Error!", exception);
});

[michael-hawker]

@powerdude we had talked about this as well, but figure if you're going to subscribe to an event, at that point you can still just subscribe to the TaskScheduler.UnobservedException event. Though I suppose in your situation you can funnel different commands to different handlers specific for that command.

@Sergio0694 I think the main problem statement it'd be good to bubble up in the description of the issue is that we want to provide option around not causing instability in applications on unexpected behaviors vs. control of program execution, right?

Also, if we have both options able to be set in the flag (vs. having a default with flag being override), will setting both flags cause an exception in the constructor?

[Arlodotexe]

I just want to throw out that since AsyncRelayCommandOptions is a flag, the long name will make many combinations almost too verbose.

[ICommand(Options = AsyncRelayCommandOptions.AllowConcurrentExecutions | AsyncRelayCommandOptions.FlowExceptionsToThreadPool | AsyncRelayCommandOptions.FlowExceptionsToSynchronizationContext)]
private async Task DoSomethingAsync()
{
    // ...
}

We might want think about a shorter name, such as AsyncOptions.

[Sergio0694]

This is why we said the attribute would use individual properties, no?

[ICommand(AllowConcurrentExecutions = true, FlowExceptionsToThreadPool = true)]
private async Task DoSomethingAsync()
{
    // ...
}

[TheCodeTraveler]

the long name will make many combinations almost too verbose

This isn't necessarily my choice for the solution to the verbosity, but I wanted to at least post the idea here since we had to do something similar to this in CommunityToolkit.Maui.Markup.

Devs can leverage using static to help reduce the verbosity:

using static CommunityToolkit.Mvvm.Input.AsyncRelayCommandOptions;


// Able to drop `AsyncRelayCommandOptions.` from the enum syntax
[ICommand(Options = AllowConcurrentExecutions | FlowExceptionsToThreadPool | FlowExceptionsToSynchronizationContext)]
private async Task DoSomethingAsync()
{
    // ...
}

For those curious, here's how we document + recommend it via the CommunityToolkit.Maui.Markup docs: https://github.com/MicrosoftDocs/CommunityToolkit/blob/cbba7bbe83206dd4fd0c2a1d2093a3dc8419955d/docs/maui/markup/extensions/grid-extensions.md?plain=1#L19-L23

[Arlodotexe]

This is why we said the attribute would use individual properties, no?

I thought we landed on the opposite, we were creating a flags Enum to help with the over-verbosity and excessive number of constructor overloads?

[Sergio0694]

That was in the constructors of the type, not in the attribute:

• Constructors would just take the options.
• The attribute will use named properties for every option, like it does today.

This solves the verbosity issue and also makes it consistent with additional flags the attribute would have.
For instance, IncludeCancelCommand, which could not be an options flag anyway as it's just for source generation.