Caliburn-Micro
Consolidate error handling across different sorts of Actions
Currently an exception thrown in a coroutine (a method returning either IEnumerable<IResult>, Task or Task<>) when invoked through Message.Attach has it's exceptions handled in Coroutine, any exceptions would appear in Coroutine.Completed.
This also affects the event aggregator with IHandleWithCoroutine and IHandleWithTask.
I consider this a potential issue because of the following
// Exception raised in Application.UnhandledException.
public void SampleMethod() { throw new InvalidOperationException(); }
// Exception raised in Coroutine.Completed
public Task SampleMethod() { throw new InvalidOperationException(); }
We could resolve this in two ways.
- Have
Coroutine.Completedraise an unhandled exception event. - Have
Coroutine.Completedthrow an exception on error. - Do nothing but add some documentation.
If there is any change I'd like to get this into 3.0.0 as it suddenly cause new exceptions if they were previously being swallowed.
Thoughts?
Since exception in option 2 causes unhandled exception event, there is no real difference between first two for me. I'm against option 3 as you have pretty nice solution (1 or 2) and nobody reads documentation.
Yeah they should both cause the same outcome, it's about the implementation and any side affects.
I know some people (myself included) have hooked into the coroutine pipeline to add features from the old Caliburn such as filters. This would impact them pretty heavily.
Interestingly we provide Coroutine.ExecuteAsync that will throw an exception if the coroutine fails. This method isn't used anywhere in the framework and is just a helper method.
Will definitely want to be consistent across the two approaches here.
This one is proving a real pain to implement in a well designed manner. Having Coroutine.Completed throw an exception if there is an error won't work because SequentialResult catches it and swallows it by sending it back through the Completed events. All becomes rather complicated when Coroutine.ExecuteAsync becomes involved. Back to the drawing board.
The more I think about this the more split I become on what the best solution could be. Not in terms of implementation but outcome. It might make more sense to leave the Task alone and let it fall to TaskScheduler.UnobservedException. Part of the reason I don't like this is we'd now have three different events for three different types of method errors.
Going to leave this here for discussion on potential ideas on best way to consolidate error handling in Caliburn.Micro.
Just posted a reply on the question, while it doesn't look to be caused by this it certainly has something to consider when doing this work.
Ideally I think making the error handling separate from any current event means we can pipe all of the errors to the same place.
