Correct usage of return Task.FromException

Question

I recently observed a code review between two developers.

The following code was submitted:

 public async Task<List<Thing>> GetThings()
    {
        try
        {
            var endpoint = $"{Settings.ThingEndpoint}/things";
            var response = await HttpClient.GetAsync(endpoint);
            return JsonConvert.DeserializeObject<List<Thing>>(await response.Content.ReadAsStringAsync());
        }
        catch (Exception e)
        {
            Log.Logger.Error(e.ToString());
            return await Task.FromException<List<Thing>>(e);
        }
    }

Which received the following review comments:

There is absolutely no need to return await Task.FromException>(e), this is something you do when dealing with non awaited task. In this case the catch will capture whatever exception var response = await HttpClient.GetAsync(endpoint); will throw. You should just remove it and catch the exception as is

I do not fully understand why not to use Task.FromException in this case, so i have the following Questions:

1. What is the reviewer saying?
2. Is the reviewer correct?
3. Why not return await Task.FromException?
4. What is a correct scenario to return await Task.FromException?

Answer 1

The reviewer is entirely correct.

The only situation you would use Task.FromException is when you're in a method you cannot or won't implement using async and await, and you want the result of the task should be an exception.

Idiot example but anyway:

public Task<int> NotReallyAsync()
{
    if (new Random().Next(2) == 0)
        return Task.FromResult(42);

    return Task.FromException<int>(new InvalidOperationException());
}

So let's deal with your questions one by one:

1. The reviewer is saying that Task.FromException should only be used in a non-async/await method, in an async/await method, you should instead just rethrow the exception:

   catch (Exception e)
   {
       Log.Logger.Error(e.ToString());
       throw;
   }
   

   or if you implement an exception filter:

   catch (Exception e) when (Log.Logger.ExceptionFilter(e)) { }
   

2. Yes, the reviewer is correct.

3. Because it is unnecessary, instead just rethrow the exception. If you want to throw an exception, just throw it. The purpose of async/await is to be able to write your method in a normal manner, so write a normal throw statement or a normal catch-block.
4. Non-async/await methods, and only that.

Answer 2

1. In general returning from catch is not good coding practice.

2. Task.FromException is generally used when you want to rely on status of Task if a known failure condition is met. For example, if an object is null you know you should return a faulted task.Client can use state of task as faulted to show appropriate message to user.I modified the code just to tell you on example.

        public async Task<List<Thing>> GetThings()
       {
           try
           {
               var endpoint = $"{Settings.ThingEndpoint}/things";
               var response = await HttpClient.GetAsync(endpoint);
               var obj = JsonConvert.DeserializeObject<List<Thing>>(await response.Content.ReadAsStringAsync());
               if(obj==null)
               {
                 return await Task.FromException<List<Thing>>(new NullRefernceException());
               }
               else
               {     
   
               }
   
           }
           catch (Exception e)
           {
               Log.Logger.Error(e.ToString());
               throw;
   
           }
       }