Correct usage of return Task.FromException

user_976537 0
Viewed 7711

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?
task .net-4.0 .net c#
2 Answers

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.
user_2670
  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;

        }
    }
user_22142300
Related