5
\$\begingroup\$

I have a simple ASP.NET Web API project. Now I want to make all basic methods as asynchronous. I tried to do something, and it's working but I'm not sure that it was implemented properly.

Please check whether I properly implemented async/await in my project.

I had implemented methods like below in my repository:

        public T GetById(object id)
        {
            return this.Entities.Find(id);
        }
        public void Insert(T entity)
        {
            try
            {
                if (entity == null)
                {
                    throw new ArgumentNullException("entity");
                }
                this.Entities.Add(entity);
                this.context.SaveChanges();
            }
            catch (DbEntityValidationException dbEx)
            {
                foreach (var validationErrors in dbEx.EntityValidationErrors)
                {
                    foreach (var validationError in validationErrors.ValidationErrors)
                    {
                        errorMessage += string.Format("Property: {0} Error: {1}",
                        validationError.PropertyName, validationError.ErrorMessage) + Environment.NewLine;
                    }
                }
                throw new Exception(errorMessage, dbEx);
            }
        }

I tried to create async methods:

        public Task InsertAsync(T entity)
        {
            return Task.Run(() => Insert(entity));
        }
        public Task<T> GetByIdAsync(object id)
        {
            return Task.Run(() => GetById(id));
        }

And now I'm using it as:

      public async virtual Task<HttpResponseMessage> Post([FromBody]T entity)
      {
                try
                {
                    await repository.InsertAsync(entity);
                    return Request.CreateResponse(HttpStatusCode.Created, entity);             
                }
                catch (Exception ex)
                {
                    return ErrorMsg(HttpStatusCode.InternalServerError, ex.Message);
                }

       }
       public async virtual Task<HttpResponseMessage> GetById(int id)
       {
            var entity = await repository.GetByIdAsync(id);

            if (entity == null)
            {                
                var message = string.Format("No {0} with ID = {1}", GenericTypeName, id);
                return ErrorMsg(HttpStatusCode.NotFound, message);
            }
            return Request.CreateResponse(HttpStatusCode.OK, entity);
       }
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! Can you explain a bit about what your script should do and if it's working? It's unclear from your post whether you want people to test that it's functioning or to tell you if you're writing code the way you should. \$\endgroup\$ Sep 7, 2015 at 10:40
  • 1
    \$\begingroup\$ @SuperBiasedMan Thank you for advice. I've just edited my question \$\endgroup\$ Sep 7, 2015 at 10:45

1 Answer 1

3
\$\begingroup\$

Your implementation is technically correct, but...

There is no benefit in using Task.Run in an web application.

All you're doing is deferring work from one thread to another. So, in fact, not only is there no benefit, you're actually adding overhead.

I'll quote Stephen Cleary, because I could nevet put it better:

There are (at least) four efficiency problems introduced as soon as you use await with Task.Run in ASP.NET:

  • Extra (unnecessary) thread switching to the Task.Run thread pool thread. Similarly, when that thread finishes the request, it has to enter the request context (which is not an actual thread switch but does have overhead).
  • Extra (unnecessary) garbage is created. Asynchronous programming is a tradeoff: you get increased responsiveness at the expense of higher memory usage. In this case, you end up creating more garbage for the asynchronous operations that is totally unnecessary.
  • The ASP.NET thread pool heuristics are thrown off by Task.Run “unexpectedly” borrowing a thread pool thread. I don’t have a lot of experience here, but my gut instinct tells me that the heuristics should recover well if the unexpected task is really short and would not handle it as elegantly if the unexpected task lasts more than two seconds.
  • ASP.NET is not able to terminate the request early, i.e., if the client disconnects or the request times out. In the synchronous case, ASP.NET knew the request thread and could abort it. In the asynchronous case, ASP.NET is not aware that the secondary thread pool thread is “for” that request. It is possible to fix this by using cancellation tokens, but that’s outside the scope of this blog post.

So, unless you're doing true async I/O, do it synchronously.

Alternatively, if you want to expose an async interface, where the implementation may or may not perform asynchronous I/O, synchronous implementation should use Task.FromResult instead of Task.Run. Example:

interface IRepository<T>
{
     public Task<T> GetByIdAsync(object id)
}

// implementation that uses synchronous I/O
class SyncRepository<T> : IRepository<T>
{
    public Task<T> GetByIdAsync(object id)
    {
        T t = this.Entities.Find(id);
        return Task.FromResult(t);
    }
}

// implementation that uses async I/O
class AsyncRepository<T> : IRepository<T>
{
    public Task<T> GetByIdAsync(object id)
    {
        return this.Entities.FindAsync(id);
    }
}

Read also:

\$\endgroup\$
6
  • \$\begingroup\$ Thanks a lot. I have a few questions: Is it correct that two methods sync. and async have the same name that containt Async? Where is the better place to add async/await, to repository methods or to controller? \$\endgroup\$ Sep 7, 2015 at 19:36
  • \$\begingroup\$ @MegaTron I'm sorry, I didn't fully understand your question. What is the exact name of these two methods? Is either one of them a wrapper for the other? As for your second question, you should make your repository async if, and only if, you're using true async I/O, as explained in the answer above. If any layer is async, everything above it will also need to be async, including the controller \$\endgroup\$
    – dcastro
    Sep 7, 2015 at 19:41
  • 1
    \$\begingroup\$ @MegaTron That was supposed to be an example of a low level API that uses true async I/O - it was indeed a bad example, I've replaced it. \$\endgroup\$
    – dcastro
    Sep 8, 2015 at 20:58
  • 1
    \$\begingroup\$ @MegaTron Again, that was pseudo-code. I don't have any experience using EF, but, after a quick google search it seems that EF does support async I/O. You could simply use FindAsync instead of Find and SaveChangesAsync instead of SaveChanges. \$\endgroup\$
    – dcastro
    Sep 8, 2015 at 21:11
  • 2
    \$\begingroup\$ @MegaTron It's my fault for posting pseudo-code - CodeReview is about real code. I've updated my answer again. Hope it helps. \$\endgroup\$
    – dcastro
    Sep 8, 2015 at 21:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.