Contract agreement when implementing a method that returns a Task

Question

Is there a MS "best practice" or contract agreement when implementing a method that returns a Task in regards to throwing exceptions? This came up when writing unit tests and I was trying to figure out if I should to test/handle this condition (I recognize that the answer could be "defensive coding", but I don't want that to be the answer).

i.e.

1. Method must always return a Task, which should contain the thrown Exception.

2. Method must always return a Task, except when the method supplies invalid arguments (i.e. ArgumentException).

3. Method must always return a Task, except when the developer goes rogue and does what he/she wants (jk).

Task Foo1Async(string id){
  if(id == null){
    throw new ArgumentNullException();
   }

  // do stuff
}

Task Foo2Async(string id){
  if(id == null){
    var source = new TaskCompletionSource<bool>();
    source.SetException(new ArgumentNullException());
    return source.Task;
  }

  // do stuff
}

Task Bar(string id){
  // argument checking
  if(id == null) throw new ArgumentNullException("id")    

  try{
    return this.SomeService.GetAsync(id).ContinueWith(t => {
       // checking for Fault state here
       // pass exception through.
    })
  }catch(Exception ex){
    // handling more Fault state here.
    // defensive code.
    // return Task with Exception.
    var source = new TaskCompletionSource<bool>();
    source.SetException(ex);
    return source.Task;
  }
}

Answer 1

I've asked a somewhat similar question recently:

Handling exceptions from the synchronous part of async method.

If the method has an async signature, it doesn't matter if you throw from the synchronous or asynchronous part of method. In both cases, the exception will be stored inside the Task. The only difference is that the resulting Task object will be instantly completed (faulted) in the former case.

If the method doesn't have async signature, the exception may be thrown on the caller's stack frame.

IMO, in either case the caller should not make any assumption about whether the exception has been thrown from the synchronous or asynchronous part, or whether the method has async signature, at all.

If you really need to know if the task has completed synchronously, you can always check its Task.Completed/Faulted/Cancelled status, or Task.Exception property, without awaiting:

try
{
    var task = Foo1Async(id);
    // check if completed synchronously with any error 
    // other than OperationCanceledException
    if (task.IsFaulted) 
    {
        // you have three options here:

        // 1) Inspect task.Exception

        // 2) re-throw with await, if the caller is an async method 
        await task;

        // 3) re-throw by checking task.Result 
        // or calling task.Wait(), the latter works for both Task<T> and Task 
    }
}
catch (Exception e)
{
    // handle exceptions from synchronous part of Foo1Async,
    // if it doesn't have `async` signature 
    Debug.Print(e.ToString())
    throw;
}

However, normally you should just await the result, without caring if the task has completed synchronously or asynchronously, and which part has possibly thrown. Any exception will be re-thrown on the caller context:

try
{
    var result = await Foo1Async(id); 
}
catch (Exception ex)
{
    // handle it
    Debug.Print(ex.ToString());
}

This works for unit testing too, as long as the async method returns a Task (the Unit Test engine doesn't support async void method, AFAIK, which makes sense: there is no Task to keep track of and await).

Back to your code, I'd put it this way:

Task Foo1Async(string id){
  if(id == null) {
    throw new ArgumentNullException();
   }

  // do stuff
}

Task Foo2Async(string id) {
  if(id == null){
    throw new ArgumentNullException();
   }

  // do stuff
}

Task Bar(string id) {
  // argument checking
  if(id == null) throw new ArgumentNullException("id")    
  return this.SomeService.GetAsync(id);
}

Let the caller of Foo1Async, Foo2Async, Bar deal with the exceptions, rather than capturing and propagating them manually.

Answer 2

I know that Jon Skeet is a fan of doing precondition-style checks in a separate synchronous method so that they are thrown directly.

However, my own opinion is "it doesn't matter". Consider Eric Lippert's exception taxonomy. We all agree that exogenous exceptions should be placed on the returned Task (not thrown directly on the caller's stack frame). Vexing exceptions should be completely avoided. The only types of exceptions in question are boneheaded exceptions (e.g., argument exceptions).

My argument is that it doesn't matter how they're thrown because you shouldn't write production code that catches them. Your unit tests are the only code that should be catching ArgumentException and friends, and if you use await then it doesn't matter when they're thrown.