Search code examples
c#asynchronousasync-awaittask

Is it possible to avoid `return await` in branches that delegate to a Task with the same return type?


I have an asynchronous method to handle aggregated requests. It basically breaks up the requests into single ones, handled by another method with the same return type, and combines the answers into a single response. In case the aggregate request actually consists only of a single request, I can delegate directly to the second method.

When I do this, since my method is async, I can't return the Task<string> from the other method directly, I need to await it, even though my method doesn't do anything with the result. On the other hand, in the branch that aggregates the responses, I really do need to get the results in order to generate the aggregated response, so I do need to await there.

Having to use return await seems unnatural to me and would seem to deprive the caller of the chance to do any optimizing of its own.

My questions are:

  1. Is it possible/recommended to avoid return await in the branch that delegates to a Task with the same return type? For example by not making the method async, and waiting the Tasks in the second branch using task.GetAwaiter().GetResult()?

  2. Or should I not worry about it because there will never be any noticeable difference in the final result for the caller?

Eventually the result will have to be awaited after all.

This example has been simplified to use simple string operations to demonstrate the idea; in reality my method is significantly more complex.

public async Task<string> ProcessCombinedRequest(List<string> requestList)
{
    if (requestList.Count == 1)
    {
        return await ProcessSingleRequest(requestList[0]);
    }

    // Start all the sub-tasks concurrently
    var taskList = new List<Task<string>>();
    foreach (string request in requestList)
    {
        taskList.Add(ProcessSingleRequest(request));
    }

    // Wait for each of them to finish and get their results
    var aggregatedResponse = new StringBuilder();
    foreach (var task in taskList)
    {
        string singleResponse = await task;
        aggregatedResponse.AppendLine(singleResponse);
    }
    return aggregatedResponse.ToString();
}

private async Task<string> ProcessSingleRequest(string request)
{
    // Long running operation
    await Task.Delay(4000);
    return request + " was processed.";
}

Edit: changed Thread.Sleep to await Task.Delay as suggested by @TheodorZoulias to improve the example.


Solution

  • Yes, but there are caveats and nuances. You can use a non-async method:

    public Task<string> ProcessCombinedRequest(List<string> requestList)
    {
        // TODO: consider ternary conditional or switch expression, lambda, etc
        // TODO: consider the empty list case?
        if (requestList.Count == 1)
        {
            return ProcessSingleRequest(requestList[0]);
        }
        return ProcessCombinedRequestMulti(requestList);
    }
    private async Task<string> ProcessCombinedRequestMulti(List<string> requestList)
    {
        // Start all the sub-tasks concurrently
        var taskList = new List<Task<string>>();
        // .. etc as before
    }
    

    This removes a layer of state-machine overhead, but importantly it also changes the behaviour in the case of exceptions - for example, if requestList turns out to be null, or some other random error happens - instead of returning a faulted Task<string>, your method may throw - and in either case, the observed stacktrace may be subtly different.

    If you're confident that there is no risk of this (perhaps with some Debug.Assert checks etc), and the non-async version is likely to be very common, and the performance matters: this is something you can certainly consider. However, if you're not confident of the impact: maybe just don't worry about it...

    To give a concrete example of this: https://github.com/dotnet/runtime/blob/ea9d53e694671ca8c70d61eb1df8779959045af6/src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Hybrid/HybridCache.cs#L74-L82