Mvvm icon indicating copy to clipboard operation
Mvvm copied to clipboard

AsyncCommand exceptions and Execution

Open uecasm opened this issue 6 years ago • 1 comments

Just found these classes and they look interesting. However I'm a little puzzled by some specific behaviour that seems contradictory.

In AsyncCommand.ExecuteAsync it explicitly rethrows the cancel/fault state of the original task, such that it propagates out of the ICommand. According to #6 and #17, this change was intentional to make it more like synchronous ICommand implementations, which is fair enough -- at first.

The thing is that if the UI is bound to the command then there is no defence against this becoming an unhandled exception.

With synchronous code, you can put the appropriate logic into your DelegateCommand (or whatever) handler, eg. a try/catch with appropriate actions for cancel and fault.

With asynchronous code, you can do the same thing -- but now you have defeated the purpose of the Execution property -- if you handle the exception at this point then Execution.IsFaulted will never become true and so any UI data-binding you've done to show an error state is useless. If you don't handle the exception at that point, then Execution.IsFaulted is correct but the exception gets rethrown somewhere that you can't catch it again. (Similarly for cancel.)

There's a little bit of a catch-22 -- if you do swallow exceptions and rely on people binding to Execution then people might be confused if they forget to do that and the exception just "vanishes". But if you don't, then Execution itself seems useless because it will never report anything other than success (or you get unhandled exceptions).

Perhaps the model to consider here is not ICommand, but AsyncOperation/BackgroundWorker. These are async and they do swallow exceptions (by reporting them in the RunWorkerCompleted handler and nowhere else). It seems to me that AsyncCommand.Execution better fits that model (since Task itself is similar).

Either that or there needs to be some way to inject some user-specified exception handling code around that await but still inside AsyncCommand.ExecuteAsync.

uecasm avatar Jun 06 '18 07:06 uecasm

A similar problem is that if you link a CancelCommand to an AsyncCommand, because the former suppresses the OperationCanceledException it results in AsyncCommand.Execution.IsCanceled never becoming true.

Removing the exception swallowing from CancelCommand (and also removing the rethrow in AsyncCommand as above) makes it behave as expected.

uecasm avatar Jun 06 '18 07:06 uecasm