Function still async can it be refined?

Question

I have this function:

public async Task<string> EagerLoadAllAsync<T>(params Expression<Func<T, object>>[] includeProperties) where T : class
{
    var entities = await _repository.EagerLoadAllAsync(includeProperties);
    entities.ForEach(l =>
    {
        var lead = l as Lead;
        if (lead.User != null) 
        {
            // We must reduce the amount of data being sent to the client side
            lead.User = new Domain.Identities.ApplicationUser { FirstName = lead.User.FirstName, LastName = lead.User.LastName, UserName = lead.User.UserName };
        }
    });

    var json = await Task.Factory.StartNew(() => JsonConvert.SerializeObject(entities, Formatting.Indented, new JsonSerializerSettings { 
        ReferenceLoopHandling = ReferenceLoopHandling.Ignore
    }));

    return json;
}

I think it is still async it has to awaitable functions running, one to fetch the data from the database and the other to convert it json.

I was wondering if the ForEach loop in the middle sort of destroys the async from the ground up approach?

Is there a way to make this more async? Should it be more async?

Before I needed to reduce the data being sent to the client I had this function:

public async Task<string> EagerLoadAllAsync<T>(params Expression<Func<T, object>>[] includeProperties) where T : class
{   
    var json = await Task.Factory.StartNew(async() => JsonConvert.SerializeObject(await await _repository.EagerLoadAllAsync(includeProperties), Formatting.Indented, new JsonSerializerSettings { 
        ReferenceLoopHandling = ReferenceLoopHandling.Ignore
    }));

    return json.Result;
}

Answer 1

Stephan cleary has a great blog post about the dangers of using Task.Run in ASP.NET:

The reason is that the ASP.NET runtime has no idea that you’ve queued this work (using Task.Run), so it’s not aware that the background work even exists. For a variety of reasons, IIS/ASP.NET has to occasionally recycle your application. If you have background work running when this recycling takes place, that work will mysteriously disappear.

When i create an async method around some asynchronous operation (DB query, web request, etc..) i stick to the rule that says "an async method should hit its await statement as soon as possible", that is because any code prior to the await will run synchronously. You may choose to use ConfigureAwait(false) to avoid returning to your request context, but that usually happends very quickly. See Best practice to call ConfigureAwait for all server-side code for more on that.

About the ForEach, i would definitely benchmark that part to see how significant of an impact it has on the async call. Other then that, read up eric lipperts post about ForEach vs foreach to see why you shouldn't use it.

I would definitely pass on spinning a new ThreadPool thread just for the sake of the JSON deserialization. It will cost you more to invoke that thread then to run it synchronously. If your JSON isnt huge, go with the sync approach on that.

Answer 2

Typically you await tasks that are relatively long-running like I/O. So, awaiting your call to the repository fetch is a good thing. Your ForEach loop isn't bad at all, but it would probably perform better if your repository fetch only returned the data you actually needed as it wouldn't have return all that data.

I'm not sure if spinning up another task to convert the entities into json is doing you any good, though.

And then we should consider this is web calls and await is really just freeing up the request handler so that it might not make each individual call faster, it just won't be tying up a request that is sitting there waiting for database I/O.

You can always put some Stopwatches in to see how long things are taking.