Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IHandleWithTask.Handle - Exceptions are never caught #222

Closed
keithdv opened this issue Aug 31, 2015 · 6 comments
Closed

IHandleWithTask.Handle - Exceptions are never caught #222

keithdv opened this issue Aug 31, 2015 · 6 comments

Comments

@keithdv
Copy link

keithdv commented Aug 31, 2015

Environment: WPF, .NET 4.5, Caliburn.Micro 2.0.2, Visual Studio 2013

Summary: Exceptions thrown within a task within an IHandleWithTask.Handle method do not get raised in any of the expected event handlers including TaskScheduler.UnobservedTaskException, BootstrapperBase.OnUnhandledException or Application.DispatcherUnhandledException. In fact there doesn't seem to be any way to get the exception with the default behavior.

The EventAggregator appears to call void Handle and Task Handle with the same logic:

var result = pair.Value.Invoke(target, new[] { message });
if (result != null) {
HandlerResultProcessing(target, result);
}

The default HandlerResultProcessing in BootstrapperBase does nothing to deal with Task.Exception. The only way I have found to get the exceptions raised is to define a new action in HandlerResultProcessing. For example:

        var a = EventAggregator.HandlerResultProcessing;

        EventAggregator.HandlerResultProcessing = (o, r) =>
        {
            if (a != null) { a(o, r); }

            var taskResult = r as Task;

            if (taskResult != null)
            {
                taskResult.ContinueWith(r1 =>
                {
                    if (r1.Exception != null)
                    {
                        throw r1.Exception;
                    }
                });
            }

        };

Question #1: Is this the expected behavior?

Per Stephen Cleary in https://msdn.microsoft.com/en-us/magazine/JJ991977.aspx an exception in an 'async void' method will be caught and raised by the SynchronizationContext. So it seems better to use IHandle with a method signature of async void IHandle. This is after all an event handler. This goes along the lines of 'async void OnInitialize' in this article: http://caraulean.com/blog/2013/07/15/using-caliburn-micro-with-async-await/.

Question #2: Since IHandle and IHandleWithTask are called in the same way and IHandleWithTask does not deal with exceptions why have IHandleWithTask? Why not just use 'async void IHandle<>.Handle' methods?

I have a stand alone project I can send if requested.

@nigel-sampson
Copy link
Contributor

At the moment both actions returning Task invoked via the normal methods and with IHandleWithTask are put through the normal Coroutine pipeline (something from before the async / await days) and also the async void behavior has changed over time (initial versions of WinRT just crashed the process).

This does mean that errors in either will show up in the Coroutine.Completed event.

I think the problem is more that this event by default logs but swallows the exception rather than raising it via the normal methods. This would be a good thing to change soon while we're still doing the 3.0.0 beta since it would be a breaking change.

In terms of your second question, while the behavior will be mostly identical having a separate interface allows customization like your example above.

@nathan-alden-sr
Copy link

Hi, Nigel. I just ran into this exact issue. Was this ever addressed? The use case here is that I have the UI thread handling a message, showing a busy indicator in preparation for a long-running operation, then starting that operation on a separate thread using Task.Run:

public async Task Handle(DatabaseInitializationStartedMessage message)
{
    BusyContent = "Initializing database...";
    IsBusy = true;

    await Task.Run(() =>{ /* Long-running operation here that could throw exceptions and should be handled by the bootstrapper's OnUnhandledException method */ });
}

As @keithdv mentioned, the only way I've gotten this to work is by using async void, which I always like to avoid using if possible. Is there another way you can recommend to perform this type of work?

@srollinet
Copy link

Same problem for me, I use async action methods in my view models and I would like to centralize the exception handling for the unexpected cases (log the exception, display a warning, etc).

I cannot return void for testability reason

@nigel-sampson
Copy link
Contributor

I've been thinking about this a bit lately. All errors are being surface somewhere right now. Just not always where we expect.

If you're returning Task from an action or using IHandleWithTask then it gets executed with the coroutine pipeline and any errors show up in the Coroutine.Completed event.

Actions that return void (including async void) are handled through the normal error handling mechanisms (usually Application.UnhandledException or whatever variant based on the platform).

Most platforms don't let us invoke that event so if we wanted to insert a global error handling part for Caliburn applications it would have to be of our own creation and pipe both the two places above into it.

Thoughts?

@nathan-alden-sr
Copy link

nathan-alden-sr commented Aug 25, 2016

Since Caliburn Micro library code is already wrapping most of the logic in most applications that use it, it would make sense for the framework to expose a single event handler that reports any capturable errors. There will still be errors that could be thrown where the stack trace doesn't include Caliburn Micro (e.g., view code-behind), but those can already be handled by existing .NET events. What I'd love to see is that, at least as far as Caliburn Micro's concerned, we get a single event handler for any exceptions that Caliburn Micro can catch.

An interesting aside: All I do when I catch an exception is display an ErrorViewModel that has a public Exception Exception property and renders the ToString() of that to a text box. The view has a Copy to Clipboard button and a Close Application button. It's pretty easy for me to wire this up in several places, but since I'm using a Caliburn Micro view model to display exceptions, I'd prefer the event handler to be within Caliburn Micro.

@nigel-sampson
Copy link
Contributor

Created #362 as a point for doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants