1

I'm using Asp.Net 4.0.

I also have a HashSet from which I want to create and run tasks asynchronously, waiting for them all to finish with a timeout applied. The code below is what I have so far, but given that there are so many ways to skin a cat with TPL, can this be improved on, or are there any critical issues?

        TaskWrapper[] taskWrappers = new TaskWrapper[nodeCount];
        Task<ResponseWrapper>[] tasks = new Task<ResponseWrapper>[nodeCount];

        int i = 0;
        foreach (var n in rt.Nodes)
        {
            tasks[i] = Task<ResponseWrapper>.Factory.StartNew(() => MyMethod.Submit(n.Data, timeout));

            taskWrappers[i].LeadPointID = n.LeadPointID;
            // other wrapper stuff;
            taskWrappers[i].Task = tasks[i];

            i++;
        }

        Task.WaitAll(tasks, timeout);

Having read the article kindly provided, it seems like this kind of code structure would be the preferred method of working:

class MyService2
{
    public int CalculateMandelbrot()
    {
        // Tons of work to do in here!
        for (int i = 0; i != 10000000; ++i)
            ;
        return 42;
    }
}

private async void MyButton_Click(object sender, EventArgs e)
{
  await Task.Run(() => myService.CalculateMandelbrot());
}

However, as I've previously pointed out I don't have access to VS2012, so given that as a starting point, is there a VS2010 equivalent to this pattern?

dotnetnoob
  • 10,783
  • 20
  • 57
  • 103
  • If everything is working as required this feels like it should be on codereview.stackexchange.com. – Daniel Kelley Jul 22 '14 at 09:59
  • With C# 4(which comes with .NET 4.0) you should be careful with [capturing foreach loops variables](http://ericlippert.com/2009/11/12/closing-over-the-loop-variable-considered-harmful-part-one/). – Ventsyslav Raikov Jul 22 '14 at 10:03
  • Looks good. Nothing wrong with that. Especially nothing wrong with using synchronous blocking calls despite async being all the rage right now. I'd replace the imperative loop with a functional `Select` (e.g. `rt.Nodes.Select(n => ...)`). – usr Jul 22 '14 at 11:39
  • @usr But blocking on a call means that thread becomes useless for the duration of the call. You're wasting resources. And this is easily avoidable. How come there's nothing wrong with it? – dcastro Jul 22 '14 at 13:59
  • @dcastro all it does is save memory (mostly the managed stack). If you have the memory available the benefit of saving it is zero. – usr Jul 22 '14 at 14:00

1 Answers1

2

Task.WaitAll will block your main thread until all other tasks have completed. That's one less thread that could be being used to server more requests.

Instead, you should be using await Task.WhenAll. However, WhenAll does not have an overload that takes a timeout. A possible workaround is to combine it with a Task.Delay, and then wait for either all your tasks to finish or the delay task to finish:

await Task.WhenAny(Task.WhenAll(tasks), Task.Delay(timeout));

If you don't have access to VS 2012, then an alternative would be to attach a continuation task to WhenAny, and return that task. The framework will take care of the rest.

public Task<string> Post()
{
    var tasks = //...
    var continuation = Task.WhenAny(Task.WhenAll(tasks), Task.Delay(timeout))
                           .ContinueWith(whenAnyTask => 
        {
             //the rest of your code
             return "Hello";
        };

    return continuation;
}

More importantly: do not use Task.Run (or StartNew) on an ASP.NET application!

You're taking threads from the threadpool to serve a client faster, but those threads could be being used to serve more clients! So you're telling other clients "hold on a minute, i'm focusing my resources on serving this guy first". You're also introducing a lot of needless overhead.

In a web app, it's good practice to use only one thread at a time to serve a client, and use asynchronous code for async I/O only.


Edit : Aron

Each thread in .net (x86) uses 1MB of RAM or 4MB on (x64). Even a fairly beefy server with 16GB of ram would ONLY be able to therefore run 4000 threads (assuming no memory usage anywhere else). That means that IF each request runs 4 threads, you can only concurrently handle 1000 requests.

Whereas a node.js server (which uses a similar model to ASP.Net async/await) is easily able to handle over 10000 concurrent requests on a single thread (read a much smaller memory footprint).


For a more detailed explanation, see Stephen Cleary's Task.Run Etiquette Examples: Don't Use Task.Run in the Implementation.

Community
  • 1
  • 1
dcastro
  • 66,540
  • 21
  • 145
  • 155
  • I believe 'await' isn't available to me in VS2010/Asp.Net 4.0 – dotnetnoob Jul 22 '14 at 09:57
  • 1
    @dotnetnoob If you upgrade to vs2012, you could. The [Async Targetting Pack](http://stackoverflow.com/a/10344633/857807) enables async/await on .NET 4.0. Even if you don't want to upgrade, your code needs reworking. You should be doing everything synchronously, in one thread, and use tasks for async I/O only. See my updated answer. – dcastro Jul 22 '14 at 10:06
  • It may be a little clearer if you know what I want to achieve. I need to make a number of Http POST requests to a third party service for each user. Some are syncrhonous and some like this are asynchronous where I need to wait for a response. – dotnetnoob Jul 22 '14 at 10:06
  • @dotnetnoob Even so, you should not be using `StartNew`. If those HTTP requests are async (as they should), the API will return a task. Your `MyMethod.Submit` method should return that task back to the controller, and then the controller awaits that task directly. – dcastro Jul 22 '14 at 10:08
  • @dcastro I would however say instead of "You should be doing everything synchronously, in one thread, and use tasks for async I/O only." I would say that you should do everything single-thread asynchronously. The problem is that years of dogma have left us with an entire generation of developers that equates asynchronous with multi-threading. Again...see Stephen Cleary and Jon Skeet on the subject. – Aron Jul 22 '14 at 10:55
  • @Aron True, that's what I meant to say :) Thanks for the correction. – dcastro Jul 22 '14 at 10:56
  • "More importantly: do not use Task.Run (or StartNew) on an ASP.NET application!" - Could you please elaborate this. I mean what alternatives you suggest. – Adarsh Kumar Jul 22 '14 at 11:14
  • @AdarshKumar The alternative is do everything single-threaded. Read the link to Stephen Cleary's blog at the end of my answer for a more detailed explanation. – dcastro Jul 22 '14 at 11:30
  • You are assuming that the architecture of his code is capable of incorporating asynchrony. That's hard to add later. And there's nothing wrong with parallelizing existing synchronous code by using Task.Run. – usr Jul 22 '14 at 11:38
  • @dcastro - just provided an update on the main thread - perhaps you'd be good enough to comment. Thanks. – dotnetnoob Jul 22 '14 at 13:42
  • @dotnetnoob I've updated my answer. The alternative would be to return the task directly, and let the framework handle it. If you need any work done after `Task.WhenAny` completes, wrap it in a continuation task. The important thing is that you don't block on a task. – dcastro Jul 22 '14 at 13:57
  • @dcastro - thanks for that - will take a closer look. – dotnetnoob Jul 22 '14 at 14:14
  • ASP.NET requires .NET 4.5 to use `async`; the async targeting pack will make the code compile but the [results are undefined](http://blogs.msdn.com/b/webdev/archive/2012/11/19/all-about-httpruntime-targetframework.aspx). – Stephen Cleary Jul 22 '14 at 16:04