Should methods that return Task throw exceptions?

Question

The methods that return Task have two options for reporting an error:

1. throwing exception right away
2. returning the task that will finish with exception

Should the caller expect both types of error reporting or is there some standard/agreement that limits task behavior to the second option?

Example:

class PageChecker {
    Task CheckWebPage(string url) {
        if(url == null) // Argument check
            throw Exception("Bad URL");

        if(!HostPinger.IsHostOnline(url)) // Some other synchronous check
            throw Exception("Host is down");

        return Task.Factory.StartNew(()=> {
            // Asynchronous check
            if(PageDownloader.GetPageContent(url).Contains("error"))
                throw Exception("Error on the page");
        });
    }
}

Handling both types looks pretty ugly:

try {
    var task = pageChecker.CheckWebPage(url);

    task.ContinueWith(t =>
        {
            if(t.Exception!=null)
                ReportBadPage(url);
        });

}
catch(Exception ex) {
    ReportBadPage(url);
}

Using async/await may help, but is there a solution for plain .NET 4 without asynchronous support?

Answer 1

Most Task-returning methods are intended for use with async/await (and as such should not use Task.Run or Task.Factory.StartNew internally).

Note that with the common way of calling asynchronous methods, it doesn't matter how the exception is thrown:

await CheckWebPageAsync();

The difference only comes in when the method is called and then awaited later:

List<Task> tasks = ...;
tasks.Add(CheckWebPagesAsync());
...
await Task.WhenAll(tasks);

However, usually the call (CheckWebPagesAsync()) and the await are in the same block of code, so they would be in the same try/catch block anyway, and in that case it also (usually) doesn't matter.

is there some standard/agreement that limits task behavior to the second option?

There is no standard. Preconditions are a type of boneheaded exception, so it doesn't really matter how it's thrown because it should never be caught anyway.

Jon Skeet is of the opinion that preconditions should be thrown directly ("outside" the returned task):

Task CheckWebPageAsync(string url) {
  if(url == null) // argument check            
    throw Exception("Bad url");                     

  return CheckWebPageInternalAsync(url);
}

private async Task CheckWebPageInternalAsync(string url) {
  if((await PageDownloader.GetPageContentAsync(url)).Contains("error")) 
    throw Exception("Error on the page");
}

This provides a nice parallel to LINQ operators, which are guaranteed to throw exceptions "early" like this (outside the enumerator).

But I don't think that's necessary. I find the code is simpler when throwing preconditions within the task:

async Task CheckWebPageAsync(string url) {
  if(url == null) // argument check            
    throw Exception("Bad url");                     

  if((await PageDownloader.GetPageContentAsync(url)).Contains("error")) 
    throw Exception("Error on the page");
}

Remember, there should never be any code that catches preconditions, so in the real world, it shouldn't make any difference how the exception is thrown.

On the other hand, this is one point where I actually disagree with Jon Skeet. So your mileage may vary... a lot. :)

Answer 2

I was having very similar issue/doubts. I was trying to implement asynchronous methods (e.g. public Task DoSomethingAsync()) which was specified in an interface. To rephrase, the interface expects the particular function (DoSomething) to be asynchronous.

However, it turns out that the implementation could do it synchronously (and I don't think it method is going to take very long to complete either).

public interface IFoobar
{
    Task DoSomethingAsync(Foo foo, Bar bar);
}

public class Caller
{
    public async void Test
    {
        try
        {
            await new Implementation().DoSomethingAsync(null, null);
        }
        catch (Exception e)
        {
            Logger.Error(e);
        }
    }
}

Now there are four ways that I could do this.

Method 1:

public class Implementation : IFoobar
{
    public Task DoSomethingAsync(Foo foo, Bar bar)
    {
        if (foo == null)
            throw new ArgumentNullException(nameof(foo));
        if (bar == null)
            throw new ArgumentNullException(nameof(bar));

        DoSomethingWithFoobar(foo, bar);
    }
}

Method 2:

public class Implementation : IFoobar
{
    #pragma warning disable 1998
    public async Task DoSomethingAsync(Foo foo, Bar bar)
    {
        if (foo == null)
            throw new ArgumentNullException(nameof(foo));
        if (bar == null)
            throw new ArgumentNullException(nameof(bar));

        DoSomethingWithFoobar(foo, bar);
    }
    #pragma warning restore 1998
}

Method 3:

public class Implementation : IFoobar
{
    public Task DoSomethingAsync(Foo foo, Bar bar)
    {
        if (foo == null)
            return Task.FromException(new ArgumentNullException(nameof(foo)));
        if (bar == null)
            return Task.FromException(new ArgumentNullException(nameof(bar)));

        DoSomethingWithFoobar(foo, bar);
        return Task.CompletedTask;
    }
}

Method 4:

public class Implementation : IFoobar
{
    public Task DoSomethingAsync(Foo foo, Bar bar)
    {
        try
        {
            if (foo == null)
                throw new ArgumentNullException(nameof(foo));
            if (bar == null)
                throw new ArgumentNullException(nameof(bar));
        }
        catch (Exception e)
        {
            return Task.FromException(e);
        }

        DoSomethingWithFoobar(foo, bar);
        return Task.CompletedTask;
    }
}

Like what Stephen Cleary has mentioned, all of these generally works. However, there are some differences.

• Method 1 requires you to catch the exception synchronously (when calling method rather than when awaiting). If you used a continuation (task.ContinueWith(task => {})) to handle exception, the continuation would simply not run. This is similar to the example in your question.
• Method 2 actually works very well, but you will have to live with the warning or insert the #pragma suppressions. The method could end up running asynchronously, causing unnecessary context switches.
• Method 3 seems the most intuitive. There is one side-effect though - stacktrace does not show DoSomethingAsync() at all! All you could see is the caller. This could be rather bad depending on how many exceptions of the same type you are throwing.
• Method 4 is similar to method 2. You could await+catch the exception; you could do task continuations; the stacktrace doesn't have missing information. It's also running synchronously, which can be useful for very lightweight/quick methods. But... it's awfully awkward to write it this way for every method you implement.

Note that I'm discussing this from the implementation's point of view - I have no control how someone else could call my method. The aim is to implement in a way that executes normally, independent on the method of calling.