# Performance concerns for synchronous action methods

I have an ASP.NET MVC 5 application using Entity Framework with SQL Server 2016. The application is mainly basic CRUD. The boilerplate generated when creating the project in Visual Studio did not have any asynchronous action methods, so I've left it alone for now. But at this point I can't figure out if the calls should be async or not.

I've never worked with async before, but I have a rudimentary understanding of how it works with ASP.NET. This question could potentially boil down to: How large does a database have to be with how many calls to it before the async overhead is worth it?

Most of my controllers look something like this. Would I benefit from asynchrony here? If you can't determine it, how can I? I'm not sure how to "run the numbers", as I often see on Stack Overflow.

I also run into the "leave it until it becomes an issue" doctrine often, but this is going right from a test group of 4 to a group of 50 when it is finished. I cringe just thinking of the backlash I'll receive when it poops the bed on the first day. I know 50 is still a relatively small number, but the server is fairly weak.

using EDB.Database;
using EDB.Identity;
using EDB.Models;
using EDB.Utilities.Controllers;
using System.Collections.Generic;
using System.Data.Entity;
using System.Linq;
using System.Net;
using System.Web.Mvc;

namespace EDB.Controllers
{
public class ClientsController : Controller
{
private ExcDbContext db = new ExcDbContext();

// GET: Clients
[HierarchialAuth(Permissions.Clients.View)]
public ActionResult Index()
{
return View(GetRows(db.Clients.ToList()));
}

// Iterates through models in list and creates a new ClientItemViewModel for each
private IEnumerable<ClientItemViewModel> GetRows(List<ClientModel> allClients)
{

foreach (ClientModel client in allClients)
{
yield return new ClientItemViewModel(client.ID, client.Name, client.PhoneNumber, client.Email, client.PhysicianIDs.Count,
db.Physicians.Find(client.PhysicianIDs[0]).Name);
}
}

// GET: Clients/Create
[HierarchialAuth(Permissions.Clients.Create)]
public ActionResult Create()
{
return View(new ClientCreateViewModel(ControllerUtil.DropDownListPhysicians(db), ControllerUtil.DropDownListLocations(db)));
}

// POST: Clients/Create
[HttpPost]
[ValidateAntiForgeryToken]
[HierarchialAuth(Permissions.Clients.Create)]
public ActionResult Create(ClientCreateViewModel viewModel)
{
if (ModelState.IsValid)
{
ClientModel clientModel = new ClientModel();
viewModel.Unflatten(clientModel);
db.SaveChanges();
return RedirectToAction("Index");
}
viewModel.AllPhysicians = ControllerUtil.DropDownListPhysicians(db);
viewModel.AllLocations = ControllerUtil.DropDownListLocations(db);
return View(viewModel);
}

// GET: Clients/Edit/5
[HierarchialAuth(Permissions.Clients.Edit)]
public ActionResult Edit(int? id)
{
if (id == null)
{
}
ClientModel clientModel = db.Clients.Find(id);
if (clientModel == null)
{
return HttpError(HttpStatusCode.NotFound);
}
return View(new ClientEditViewModel(clientModel, ControllerUtil.DropDownListPhysicians(db), ControllerUtil.DropDownListLocations(db)));
}

// POST: Clients/Edit/5
[HttpPost]
[ValidateAntiForgeryToken]
[HierarchialAuth(Permissions.Clients.Edit)]
public ActionResult Edit(ClientEditViewModel viewModel, int id)
{
ClientModel cm = db.Clients.Find(id);
if (cm == null)
{
}
if (ModelState.IsValid)
{
viewModel.Unflatten(cm);
db.Entry(cm).State = EntityState.Modified;
db.SaveChanges();
return RedirectToAction("Index");
}
viewModel.AllPhysicians = ControllerUtil.DropDownListPhysicians(db);
viewModel.AllLocations = ControllerUtil.DropDownListLocations(db);
return View(viewModel);
}

private IEnumerable<string> GetPhysicianNames(ClientModel cm)
{
return cm.PhysicianIDs.Select(id => db.Physicians.Find(id).Name);
}
private IEnumerable<string> GetLocationNames(ClientModel cm)
{
return cm.LocationIDs.Select(id => db.Locations.Find(id).Name);
}

// GET: Clients/Details/5
[HierarchialAuth(Permissions.Clients.View)]
public ActionResult Details(int? id)
{
if (id == null)
{
}
ClientModel cm = db.Clients.Find(id);
if (cm == null)
{
return HttpError(HttpStatusCode.NotFound);
}
return View(new ClientDetailsViewModel(cm, GetPhysicianNames(cm), GetLocationNames(cm)));
}

// GET: Clients/Delete/5
[HierarchialAuth(Permissions.Clients.Delete)]
public ActionResult Delete(int? id)
{
if (id == null)
{
}
ClientModel cm = db.Clients.Find(id);
if (cm == null)
{
return HttpError(HttpStatusCode.NotFound);
}
return View(new ClientDeleteViewModel(cm, GetPhysicianNames(cm), GetLocationNames(cm)));
}

// POST: Clients/Delete/5
[HttpPost, ActionName("Delete")]
[ValidateAntiForgeryToken]
[HierarchialAuth(Permissions.Clients.Delete)]
public ActionResult DeleteConfirmed(int id)
{
ClientModel clientModel = db.Clients.Find(id);
db.Clients.Remove(clientModel);
db.SaveChanges();
return RedirectToAction("Index");
}

protected override void Dispose(bool disposing)
{
if (disposing)
{
db.Dispose();
}
base.Dispose(disposing);
}
}
}


How large does a database have to be with how many calls to it before the async overhead is worth it?

You will have benefits immediately. SQL Server is a multi-user database and even for small/simple/quick queries you will have some benefits.

Most of my controllers look something like this. Would I benefit from asynchrony here?

Don't think it will speed-up your queries, if you have a request X and a request Y executed serially then total time will be X + Y (possibly the query Y will wait the query X to complete). Doing them in parallel won't change X's execution time (on contrary it may be slightly worse) but Y doesn't need to wait X to complete and it will be executed immediately (assuming no locks are involved).

In short: for a single non concurrent query won't probably see any measurable benefit but with multiple concurrent requests you will immediately see a performance gain (which is in fact a responsiveness gain.) OK, this is not entirely true and you can disable Session State in ASP.NET to let requests be served in parallel (loosing TempData) but don't go that far now...

You don't need to make it asynchronous now if you don't have any performance problem (even if it's a relatively smooth and painless change). What you're doing wrong in your code is the way you handle DbContext. You're not handling errors and some errors may cause the connection to be invalid. Solution: do not create the connection at class level but at function level:

 [HierarchialAuth(Permissions.Clients.View)]
public ActionResult Index()
{
using (var db = new ExcDbContext())
{
return View(GetRows(db.Clients.ToList()));
}
}


It's still not the optimal version, please read Know when to retry or fail when calling SQL Server from C#? for further details about error handling.

Here we also have the opportunity to optimize you code. Currently you're fetching the entire Clients table in memory (because of .ToList()) but you do not actually need to materialize the list (which may become pretty huge). Let EF alone to decide when/if it's necessary:

     using (var db = new ExcDbContext())
{
return View(GetRows(db.Clients));
}


With:

private IEnumerable<ClientItemViewModel> GetRows(IEnumerable<ClientModel> allClients) { }


You're also using directly IEnumerable<ClientItemViewModel> as model for your view. I'd suggest to do not do it and encapsulate the list into a proper object:

public sealed class ClientsIndexViewModel
{
IEnumerable<ClientItemViewModel> Clients { get; set; }
}


If in future you will need to add any property for this view then you won't need to refactor existing code.

You should consider to add pagination and sorting, I suppose you do not want to return clients in random order and if list grows enough it may be useful to show a shorter list (especially to improve responsiveness).

In Edit(ClientEditViewModel, int) you have an id argument but if I'm not wrong the client ID is already in ClientEditViewModel then you can drop it.

You often do return HttpError(HttpStatusCode.BadRequest). I don't remember in which version they have been introduced but you can simply return BadRequest() (see also the other similar functions).

Method names (like Index()) are unlikely to change but I strongly dislike string constants, you can replace return RedirectToAction("Index"); with return RedirectToAction(nameof(Index));.

Big blocks of code are repeated in each method. You can extract a reusable function:

IActionResult Do(ExcDbContext db, int? id, Func<ClientModel, IActionResult> action)
{
if (id == null)

var cm = db.Clients.Find(id);
if (cm == null)
return NotFound();

return action(cm);
}


Used like this:

 [HierarchialAuth(Permissions.Clients.Delete)]
public ActionResult Delete(int? id)
{
using (var db = new ExcDbContext())
{
Do(db, id, cm =>
{
return View(new ClientDeleteViewModel(...));
}
}
}


You may also rewrite Do() to create an instance of the DB context and to handle all the relevant error handling/retrying logic.

I do not know your requirements but I do not see any security check, if this code can be invoked by clients (for example Edit() to update their own data) then you should also include some validation (a malicious client X cannot modify data of another client Y...)

Code like return cm.LocationIDs.Select(id => db.Locations.Find(id).Name); is pretty inefficient when DB grows because there are almost no chances for EF to translate it to SELECT Name FROM Locations WHERE Id IN (<id list>). It can be easily rewritten to:

return db.Locations.Where(x => ids.Contains(x.Id))


Where ids is your IList<int> of IDs.

• Wow. That's a thorough response. I'm calling .ToList() in my call to GetRows(List<ClientModel>), because otherwise I run into a datareader exception. An alternative is to create another DbContext, but I'm not sure that's the correct route. I don't suppose you could also show the code including the DbContext construction in the Do() method? Thank you. – Sinjai Sep 27 '17 at 18:22
• You should create a new context for each call! Connection string is cached and connection itself is pooled, overhead is absolutely minimal and you do not have synchronization issues (this is especially true when you will move to async). Examples for connecting handling are in the linked post. – Adriano Repetti Sep 27 '17 at 19:37
• I guess it is two calls, but it feels like one (the second call being a helper method). You said "You may also rewrite Do() to create an instance of the DB context and to handle all the relevant error handling/retrying logic." -- I'm not sure how to put the DbContext bit into Do(). If I put the DbContext into Do(), I don't see how I'd have access to it when I call the method in the ActionResult, which I typically need to do. And would you really call it Do()? – Sinjai Sep 28 '17 at 2:12
• And you recommend doing all this every time I access DbContext? – Sinjai Sep 28 '17 at 2:16
• I can't do this, no matter where I put the using. What's the solution here? – Sinjai Sep 28 '17 at 2:30