2
\$\begingroup\$

I'm attempting to correctly convert a synchronous controller method to asynchronous, given that the operation it performs is CPU-intensive.

private MyDbContext db = new MyDbContext();
public async Task<ActionResult> Index()
{
    List<DashboardItemViewModel> viewModels = new List<DashboardItemViewModel>();
    List<ProductModel> products = db.Products.ToList();
    await Task.Run(() =>
    {
        foreach (ClientModel client in db.Clients)
        {
            // Constructor uses lots of logic, mostly looping through the list
            //   many times
            viewModels.Add(new DashboardItemViewModel(client, products));
        }
    });
    return View(viewModels);
}
\$\endgroup\$
2
  • \$\begingroup\$ If ctor is so slow and it goes through the list multiple times then I'd post it here as well. Chances you may get some useful insight about it \$\endgroup\$ Aug 19, 2017 at 11:02
  • \$\begingroup\$ @Adriano Good advice, but the constructor is almost entirely comprised of a loop. It's actually not slow right now, but the collection is much smaller than it eventually will be. \$\endgroup\$
    – Sinjai
    Aug 21, 2017 at 16:35

2 Answers 2

1
\$\begingroup\$

I think the closures are unnecessary and create (small or not) overhead you can avoid. See Using Asynchronous Methods in ASP.NET MVC 4 where specifically the placement of the await keyword is within the return and additionally (unlike the example) you can return the entire self contained code block.

\$\endgroup\$
3
\$\begingroup\$

As a key area of concern with this code, you really should be passing a cancellation token here, as it is a long running operation.

You need to worry about the client giving up on the request, let's say they navigated to the URL and decided they didn't mean to. The operation will still continue to run until the thread operation is complete.

This kind of issue can also lead to all kinds of problems.

Note that this can also occur when there multiple requests (say an impatient user hits refresh frequently) or a lot of traffic.

\$\endgroup\$
3
  • \$\begingroup\$ Very good point. Honestly, I wasn't even sure it would run async at all. This stuff is rather confusing for a newcomer. \$\endgroup\$
    – Sinjai
    Aug 19, 2017 at 4:40
  • \$\begingroup\$ In your particular use case, I'm not convinced the implementation is correct. However, there isn't enough code / context provided for me to be sure. \$\endgroup\$
    – Svek
    Aug 19, 2017 at 4:43
  • \$\begingroup\$ The constructor is just a loop. So if there's something more to it than this, which wouldn't surprise me, I'm doing something wrong. I'm really not sure how to test if something is actually async or not. \$\endgroup\$
    – Sinjai
    Aug 21, 2017 at 16:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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