Using async BCL with async function

Question

I've flagged this as .NET 4, as I am using the async BCL.

I have the following code:

using System.Threading.Tasks;

public static async Task<ObservableCollection<MyResult>> GetMyData(Params p)
{
    DoStuffClass stuff = new DoStuffClass();
    ObservableCollection<MyResults> results = null;

    await Task.Factory.StartNew(() =>
    {
        results = stuff.LongDrawnOutProcess(p);
    });

    return results;
}

Which I have refactored from a previous version that looked something like this:

static void GetMyDataAsync(Params p, EventHandler<MyArgs> callback)
{
    DoStuffClass stuff = new DoStuffClass();
    stuff.LoadCompleted += callback;
    stuff.LongDrawOutProcessAsync(p)
}

The inspiration for this came from here.

By far, the usage of the first case is the easiest, which was why I refactored; my question is: are there any pitfalls to this method? The LongDrawnOutProcess does hit the database.

Answer 1

Rather than relying on the side effect of the task setting another variable, you should use the task's Result to access the results of a Task. While in this case you aren't accessing the results too early, it is an easy mistake to make when using this style of programming. You also need to worry about whether the appropriate memory barriers are in place such that the changed variable will be properly observed.

The more preferable method would be this:

public static async Task<ObservableCollection<MyResult>> GetMyData(Params p)
{
    DoStuffClass stuff = new DoStuffClass();

    return await Task.Factory.StartNew(() => stuff.LongDrawnOutProcess(p));
}

Next, since you're never do anything after any await besides returning a value, there's no real reason to use async here at all; you can just write:

public static Task<ObservableCollection<MyResult>> GetMyData(Params p)
{
    DoStuffClass stuff = new DoStuffClass();

    return Task.Factory.StartNew(() => stuff.LongDrawnOutProcess(p));
}

Finally, this does have a more fundamental problem. You're not using the actual asynchronous method. You're creating a thread pool thread that is just going to spend all of its time sitting around waiting for this method to complete. That is wasteful, and the design of asynchronous programming is to avoid such synchronous waits, not to just push them off to another thread. IF you want your program to be asychronous you should still use the Async method, but you can still have the benefit of converting it to a Task based style of asynchrony:

static Task<ObservableCollection<MyResult>> GetMyDataAsync(Params p)
{
    var tcs = new TaskCompletionSource<ObservableCollection<MyResult>>();
    DoStuffClass stuff = new DoStuffClass();
    stuff.LoadCompleted += (args) => tcs.TrySetResult(args.Result);
    stuff.LongDrawOutProcessAsync(p);
    return tcs.Task;
}