+972 52-548-6969
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:
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
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.
Possibly a dirty hack, but is there any issue with just handling the TaskScheduler.UnobservedTaskException event to stop your process from crashing?
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.
Ayende,
Just curious, why do you use GC.KeepAlive?
Luke, If that will NOT fail, you'll get an error about unobserved cancelled task
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
@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.
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.
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<Task> that ContinueWith would return in my example above (not that it really makes any difference in this particular case).
I use something like this:
@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 :)
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.
I think the UnobservedExcetpion event is the right solution for this. You need to log those exceptions anyway.
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.
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.
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.
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)
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.
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
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.
Comment preview