Ayende @ Rahien

Refunds available at head office

It uses async, run for the hills (On .Net 4.0)

One of the major problems in .NET 4.0 async operation stuff is the fact that an unobserved exception will ruthlessly kill your application.

Let us look at an example:

image

On startup, check the server for any updates, without slowing down my system startup time. All well and good, as long as that server is reachable.

When it doesn’t, it will throw an exception, but not on the current thread, it will be thrown on another thread, and when the task is finalized, it will raise an UnobservedTaskException. Okay, so I’ll fix that and write code like this:

CheckForUpdatesAsync().ContinueWith(task=> GC.KeepAlive(task.Exception));

And that would almost work, except the implementation of CheckForUpdateAsync is:

private static Task CheckForUpdatesAsync()
{
    var webRequest = WebRequest.Create("http://myserver.com/update-check");
    webRequest.Method = "POST";
    return webRequest.GetRequestStreamAsync()
        .ContinueWith(task => task.Result.WriteAsync(CurrentVersion))
        .ContinueWith(task => webRequest.GetResponseAsync())
        .ContinueWith(task => new StreamReader(task.Result.GetResponseStream()).ReadToEnd())
        .ContinueWith(task =>
                          {
                              if (task.Result != "UpToDate")
                                  ShowUpdateDialogToUser();
                          });
}

Note the highlighted line, where we are essentially ignoring the failure to write to the server. That task is going to go away unobserved, the result, when GC happens, you’ll have an unobserved task exception.

This sort of error has all of the fun aspects of a good problem:

  • Only happen during errors
  • Async in nature
  • Bring down your application
  • Error location and error notification are completely divorced from one another

It is actually worse than having a memory leak!

This post explains some of the changes made with regards to unobserved exceptions in 4.5, and I wholeheartedly support this, but in 4.0, writing code that uses the TPL is easy and fun, but require careful code review to make sure that you aren’t leaking an unobserved exception.

Comments

Ali Kheyrollahi
07/23/2012 09:16 AM by
Ali Kheyrollahi

Well, ContinueWith without checking the exception or accessing .Result is suicide.

I suppose that code would be safe if you had used webRequest.GetResponseAsync().Result... assuming the method returns a task.

Mark
07/23/2012 09:20 AM by
Mark

Possibly a dirty hack, but is there any issue with just handling the TaskScheduler.UnobservedTaskException event to stop your process from crashing?

Luke
07/23/2012 09:43 AM by
Luke

Or -- if you feel that TaskScheduler.UnobservedTaskException is too much of a blunt instrument -- you could deal with errors on a per-call basis by tacking something like .ContinueWith(task => LogExceptionOrSimplyIgnoreIt(task.Exception), TaskContinuationOptions.OnlyOnFaulted) as the final continuation in the chain.

Geert Baeyaert
07/23/2012 09:47 AM by
Geert Baeyaert

Ayende,

Just curious, why do you use GC.KeepAlive?

Ayende Rahien
07/23/2012 09:48 AM by
Ayende Rahien

Luke, If that will NOT fail, you'll get an error about unobserved cancelled task

Ayende Rahien
07/23/2012 09:48 AM by
Ayende Rahien

Geert, Because doing something like:

var _ = task.Exception;

Will generate a warning about unused variable. GC.KeepAlive is a no op, and it makes the compiler happy

Luke
07/23/2012 09:58 AM by
Luke

@Ayende: Good point, I didn't test before posting. The crux of it still stands though: something like .ContinueWith(task => { if (task.IsFaulted) LogExceptionOrSimplyIgnoreIt(task.Exception); return task; }) would do the trick, although it'd be preferable to wrap that into it's own extension method to keep everything clean.

Ayende Rahien
07/23/2012 10:00 AM by
Ayende Rahien

Luke, The problem? It is semantically the same as saying, what is the problem with those memory leaks? Just call free() on every alloc(), done.

Luke
07/23/2012 10:01 AM by
Luke

And, of course, if you write a custom extension method you can use TaskCompletionSource behind-the-scenes so that you can return a plain Task rather than needing to call Unwrap on the Task that ContinueWith would return in my example above (not that it really makes any difference in this particular case).

Jesús
07/23/2012 10:02 AM by
Jesús

I use something like this:

    public static Task<TResult> ObservedContinueWith<TResult>(this Task task, Func<Task, TResult> continuationFunction, TaskContinuationOptions continuationOptions)
    {
        return task.ContinueWith(t =>
        {
            if (t.Status == TaskStatus.Faulted)
            {
                throw t.Exception;
            }
            return continuationFunction(t);
        }, continuationOptions);
    }
Luke
07/23/2012 10:04 AM by
Luke

@Ayende: I agree completely. But if you want to do fire-and-forget async stuff on .NET4 then you don't have much option. Either handle UnobservedTaskException globally or handle the exceptions on a per-call basis. Although I guess that's the whole point of your original post: you need to be careful doing fire-and-forget async stuff on .NET4 :)

tobi
07/23/2012 11:21 AM by
tobi

I kind of disagree with the fact that it is ok to intentionally have unobserved exception. That should only happen due to a bug, and should be logged. It should not bring down the process you certainly don't want to ignore exceptions.

Because that is just a "catch {}" which is clearly not acceptable.

tobi
07/23/2012 11:22 AM by
tobi

I think the UnobservedExcetpion event is the right solution for this. You need to log those exceptions anyway.

tobi
07/23/2012 05:16 PM by
tobi

I want to stress that ignoring a tasks exception, disregarding the type of the exception, is equal to "catch {}". I abhor that practice because it tends to mask real bugs.

Aaron Murray
07/23/2012 06:28 PM by
Aaron Murray

This is the part where pervasive method chaining (thanks jQuery/desire for convenience) is just perpetuating bad practices in places where it has no business. That style is only prudent within a context that understands that failure handling is baked into those methods and when the return type doesn't get affected.

"Foo Bar".ToLower().Trim().ToUpper().TrimEnd() ... should never fail given the appropriate initial input (non-null string).

The example posted is just far too complicated to expect to have actually work in every case. I think Ayende's point is that the fun new convenience options were secretly masking a new very bad worst-case scenario that nobody's applications were created to handle (new exception case).

This is just poor form by the .NET team...and they've been delivering tons of great stuff for the past few years, which makes the problem harder to swallow because it hurts so bad.

Carsten
07/24/2012 04:37 AM by
Carsten

This must be why in 4.5 there is switch do silently swallow those Exceptions on the finalizer thread (well they did because of async/await) and it's even default if I remember correctly.

BTW: here is a good explanation of the thought that got into this: http://blogs.msdn.com/b/pfxteam/archive/2009/05/31/9674669.aspx I think he is not wrong there.

We are programmers: we should know our tools and their limitation.

Jerome Laban
07/24/2012 12:25 PM by
Jerome Laban

I wouldn't have said "run for the hills" only for .NET 4.0... Unfortunately, there is the similar pitfall in .NET 4.5, with async void. This can bring down the process the same way.

(Sorry for the plug, but I think it's relevant: http://www.jaylee.org/post/2012/07/08/c-sharp-async-tips-and-tricks-part-2-async-void.aspx)

Mauricio Scheffer
07/27/2012 01:54 PM by
Mauricio Scheffer

What you really want here is to bring the exceptions into the type system, modeling it as an algebraic data type like everyone else (F#, Scala) does. Then you can easily manipulate the result of each computation through a functor, applicative or monad, without any surprises. The TPL team tried to hard to avoid ADTs in their API and these are the consequences. Luckily it's quite easy to wrap exceptions into an Either type.

Ayende Rahien
07/27/2012 02:21 PM by
Ayende Rahien

Mauricio, Um, no, it isn't the case. The issue is how do you handle an exception in an async task whose return value wasn't observed. For example, what would happen here in the case of an error

        let fetchAsync (name, url:string) =
            async {
                let uri = new System.Uri(url)
                let webClient = new WebClient()
                let! html = webClient.AsyncDownloadString(uri)
                printfn "Read %d characters for %s" html.Length name
            } |> Async.Start
Mauricio Scheffer
07/27/2012 03:03 PM by
Mauricio Scheffer

Oren: what I mean is using Async.Catch ( http://msdn.microsoft.com/en-us/library/ee353899.aspx ) to wrap the exception into a Choice type. The F# async API offers this and makes exception handling really easy (even though the computation expression can already handle exceptions on its own, you can write a regular try/with within the async block to do it the imperative way). Examples: https://gist.github.com/3188428

I'm not aware of any similar function in the TPL API, probably because the TPL team didn't want to introduce a public Either (or Choice) ADT.

Yes, an unhandled exception will pop up in another thread as you explain. More reason to wrap exceptions in an Either type! :) Avoiding partial functions just makes programming in general much easier.

Comments have been closed on this topic.