112

I have the following operation in a Web API I created:

// GET api/<controller>
[HttpGet]
[Route("pharmacies/{pharmacyId}/page/{page}/{filter?}")]
public CartTotalsDTO GetProductsWithHistory(Guid pharmacyId, int page, string filter = null ,[FromUri] bool refresh = false)
{
    return delegateHelper.GetProductsWithHistory(CustomerContext.Current.GetContactById(pharmacyId), refresh);
}

The call to this webservice is done through a Jquery Ajax call this way:

$.ajax({
      url: "/api/products/pharmacies/<%# Farmacia.PrimaryKeyId.Value.ToString() %>/page/" + vm.currentPage() + "/" + filter,
      type: "GET",
      dataType: "json",
      success: function (result) {
          vm.items([]);
          var data = result.Products;
          vm.totalUnits(result.TotalUnits);
      }          
  });

I've seen some developers that implement the previous operation this way:

// GET api/<controller>
[HttpGet]
[Route("pharmacies/{pharmacyId}/page/{page}/{filter?}")]
public async Task<CartTotalsDTO> GetProductsWithHistory(Guid pharmacyId, int page, string filter = null ,[FromUri] bool refresh = false)
{
    return await Task.Factory.StartNew(() => delegateHelper.GetProductsWithHistory(CustomerContext.Current.GetContactById(pharmacyId), refresh));
}

Gotta say, though, that GetProductsWithHistory() is a quite long operation. Given my problem and context, how will making the webAPI operation asynchronous benefit me?

svick
  • 236,525
  • 50
  • 385
  • 514
David Jiménez Martínez
  • 3,053
  • 5
  • 23
  • 43
  • 1
    The client side uses AJAX, which is already asynchronous. You do not need the service to also be written as an `async Task`. Remember, AJAX was implemented before the TPL even existed :) – Dominic Zukiewicz Oct 02 '14 at 10:24
  • Valuable input from this [**thread**](https://stackoverflow.com/questions/21771219/async-and-await-are-they-bad). – urbz Oct 02 '14 at 10:27
  • 67
    You need to understand why are you implementing async controllers, many don't. IIS has a limited number of threads available and when all are in use the server can’t process new requests. With async controllers when a process is waiting for I/O to complete, its thread is freed up for the server to use for processing other requests. – Matija Grcic Oct 02 '14 at 10:30
  • 3
    What developers have you seen do that? If there's any blog post or article that recommends that technique, please do post a link. – Stephen Cleary Oct 02 '14 at 11:53
  • 3
    You only get the full benefit of async if your process is async-aware from the top (including the web application itself and your controllers) down to any waitable activities going outside your process (including timer delays, file I/O, DB access, and web requests it makes). In this case, your delegate helper needs a `GetProductsWithHistoryAsync()` returning `Task`. There can be a benefit to writing your controller async if you intend to migrate the calls it makes to be async, too; then you start getting benefit from the async parts as you migrate the rest. – Keith Robertson Jun 18 '15 at 17:33
  • In your case, with that sample, this is not reasonable. But if you have independent processes (more than one), you can make controller action async, because you can process independent processes simultaneously. In that case you should made those independent processes async too... – efaruk Jan 22 '16 at 08:51
  • 1
    If the process you're doing is going off and hitting the database then your web thread is just waiting for it to get back and holding that thread. If you have hit your max thread count and another request comes in, it has to wait. Why do that? Instead you'd want to free that thread from your controller so another request can use it and only take up another web thread when your original request from the database came back. https://msdn.microsoft.com/en-us/magazine/dn802603.aspx – user441521 Dec 06 '16 at 19:58

2 Answers2

101

In your specific example the operation is not asynchronous at all so what you're doing is async over sync. You're just releasing one thread and blocking another. There's no reason to that, because all threads are thread pool threads (unlike in a GUI application).

In my discussion of “async over sync,” I strongly suggested that if you have an API which internally is implemented synchronously, you should not expose an asynchronous counterpart that simply wraps the synchronous method in Task.Run.

From Should I expose asynchronous wrappers for synchronous methods?

However when making WebAPI calls async where there's an actual asynchronous operation (usually I/O) instead of blocking a thread that sits and waits for a result the thread goes back to the thread pool and so able to perform some other operation. Over all that means that your application can do more with less resources and that improves scalability.

tomRedox
  • 28,092
  • 24
  • 117
  • 154
i3arnon
  • 113,022
  • 33
  • 324
  • 344
  • 3
    @efaruk all threads are worker threads. Releasing one ThreadPool thread and blocking another is pointless. – i3arnon Jan 06 '16 at 08:22
  • 1
    @efaruk I'm not sure what you're trying to say.. but as long as you agree there's no reason to use async over sync in WebAPI then it's fine. – i3arnon Jan 20 '16 at 21:08
  • @efaruk "async over sync" (i.e. `await Task.Run(() => CPUIntensive())`) is useless in asp.net. You don't gain anything from doing that. You're just releasing one ThreadPool thread to occupy another. It's less efficient than simply calling the synchronous method. – i3arnon Jan 21 '16 at 23:48
  • "async over sync" may be reasonable in UI apps where you want to free the single UI thread, so you offload work to the ThreadPool's threads. – i3arnon Jan 21 '16 at 23:49
  • 1
    @efaruk No, that is not reasonable. You example runs the independent tasks sequentially. You really need to read up on asyc/await before making recommendations. You would need to use `await Task.WhenAll` in order to execute in parallel. – Søren Boisen Jan 22 '16 at 14:36
  • 1
    @efaruk As Boisen explains, your example doesn't add any value on top of simply calling these synchronous methods sequentially. You can use `Task.Run` if you want to parallelize your load on multiple threads, but that isn't what "async over sync" means. "async over sync" references creating an async method as a wrapper over a synchronous one. You can see the in the quote in my answer. – i3arnon Jan 22 '16 at 14:44
  • I think some webapi calls using async is fine, depending on the intent of the api. If you're using a front end ajax call to the webapi method, and that method does one thing only, like "createuser", then you don't need to create an async method. But if the method has a multitude of intents, like "create user" "update user" "log user" "delete user" "append roles", with some parameter conditions, then async would be way more useful there. – sksallaj Mar 24 '16 at 16:55
  • @i3arnon I want my api controller to kick off a long running operation (mulitple database calls and processing) on a new thread and return immediately so it does not block the UI of my website. Granted that I am still using another threadpool thread, but using up all ThreadPool threads in not my concern. Mine is an internal app with light usage. Not blocking the user UI is the key requirement. I tried to spawn another thread with new db context from my Api Controller, but I get “Safe handle has been closed” error because after spawning, my main controller returns HTTP OK. Any suggestions? – Punit Vora Jun 06 '18 at 19:56
1

One approach could be (I've used this successfully in customer applications) to have a Windows Service running the lengthy operations with worker threads, and then do this in IIS to free up the threads until the blocking operation is complete: Note, this presumes results are stored in a table (rows identified by jobId) and a cleaner process cleaning them up some hours after use.

To answer the question, "Given my problem and context, how will making the webAPI operation asynchronous benefit me?" given that it's "quite a long operation" I'm thinking many seconds rather than ms, this approach frees up IIS threads. Obviously you also have to run a windows service which itself takes resource but this approach could prevent a flood of the slow queries from stealing threads from other parts of the system.

// GET api/<controller>
[HttpGet]
[Route("pharmacies/{pharmacyId}/page/{page}/{filter?}")]
public async Task<CartTotalsDTO> GetProductsWithHistory(Guid pharmacyId, int page, string filter = null ,[FromUri] bool refresh = false)
{
        var jobID = Guid.NewGuid().ToString()
        var job = new Job
        {
            Id = jobId,
            jobType = "GetProductsWithHistory",
            pharmacyId = pharmacyId,
            page = page,
            filter = filter,
            Created = DateTime.UtcNow,
            Started = null,
            Finished = null,
            User =  {{extract user id in the normal way}}
        };
        jobService.CreateJob(job);

        var timeout = 10*60*1000; //10 minutes
        Stopwatch sw = new Stopwatch();
        sw.Start();
        bool responseReceived = false;
        do
        {
            //wait for the windows service to process the job and build the results in the results table
            if (jobService.GetJob(jobId).Finished == null)
            {
                if (sw.ElapsedMilliseconds > timeout ) throw new TimeoutException();
                await Task.Delay(2000);
            }
            else
            {
                responseReceived = true;
            }
        } while (responseReceived == false);

    //this fetches the results from the temporary results table
    return jobService.GetProductsWithHistory(jobId);
}