0

I fire up some async tasks in parallel like the following example:

var BooksTask = _client.GetBooks(clientId);
var ExtrasTask = _client.GetBooksExtras(clientId);
var InvoicesTask = _client.GetBooksInvoice(clientId);
var ReceiptsTask = _client.GetBooksRecceipts(clientId);

await Task.WhenAll(
    BooksTask,
    ExtrasTask,
    InvoicesTask,
    ReceiptsTask
);

model.Books = BooksTask.Result; 
model.Extras = ExtrasTask.Result; 
model.Invoices = InvoicesTask.Result; 
model.Receipts = ReceiptsTask.Result; 

This results in a lot of typing. I searched the .Net Framework for a way to shorten this up. I imagine it to be lile this. I call the class Collector as I don't know how to name the concept.

var collector = new Collector();

collector.Bind(_client.GetBooks(clientId), out model.Books);

collector.Bind(_client.GetBooksExtras(clientId), out model.Extras);

collector.Bind(_client.GetBooksInvoice(clientId), out model.Invoices);

collector.Bind(_client.GetBooksRecceipts(clientId), out model.Receipts);

collector.Run();

Is this a valid approach? Is there something like that?

aggsol
  • 2,343
  • 1
  • 32
  • 49

3 Answers3

4

Personally, I prefer the code in the question (but using await instead of Result for code maintainability reasons). As noted in andyb952's answer, the Task.WhenAll is not required. I do prefer it for readability reasons; it makes the semantics explicit and IMO makes the code easier to read.

I searched the .Net Framework for a way to shorten this up.

There isn't anything built-in, nor (to my knowledge) any libraries for this. I've thought about writing one using tuples. For your code, it would look like this:

public static class TaskHelpers
{
    public static async Task<(T1, T2, T3, T4)> WhenAll<T1, T2, T3, T4>(Task<T1> task1, Task<T2> task2, Task<T3> task3, Task<T4> task4)
    {
        await Task.WhenAll(task1, task2, task3, task4).ConfigureAwait(false);
        return (await task1, await task2, await task3, await task4);
    }
}

With this helper in place, your original code simplifies to:

(model.Books, model.Extras, model.Invoices, model.Receipts) = await TaskHelpers.WhenAll(
    _client.GetBooks(clientId),
    _client.GetBooksExtras(clientId),
    _client.GetBooksInvoice(clientId),
    _client.GetBooksRecceipts(clientId)
);

But is it really more readable? So far, I have not been convinced enough to make this into a library.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
3

In this case I believe that the WhenAll is kind of irrelevant as you are using the results immediately after. Changing to this will have the same effect.

var BooksTask = _client.GetBooks(clientId);
var ExtrasTask = _client.GetBooksExtras(clientId);
var InvoicesTask = _client.GetBooksInvoice(clientId);
var ReceiptsTask = _client.GetBooksRecceipts(clientId);

model.Books = await BooksTask; 
model.Extras = await ExtrasTask; 
model.Invoices = await InvoicesTask; 
model.Receipts = await ReceiptsTask; 

The awaits will take care of ensuring you don't move past the 4 later assignments until the tasks are all completed

andyb952
  • 1,931
  • 11
  • 25
-1

As pointed out in andyb952's answer, in this case it's not really needed to call Task.WhenAll since all the tasks are hot and running.

But, there are situations where you may still desire to have an AsyncCollector type.


TL;DR:

async Task Async(Func<Task> asyncDelegate) =>
    await asyncDelegate().ConfigureAwait(false);
var collector = new AsyncCollector();

collector.Register(async () => model.Books = await _client.GetBooks(clientId));
collector.Register(async () => model.Extras = await _client.GetBooksExtras(clientId));
collector.Register(async () => model.Invoices = await _client.GetBooksInvoice(clientId));
collector.Register(async () => model.Receipts = await _client.GetBooksReceipts(clientId));

await collector.WhenAll();

If you're worried about closures, see the note at the end.


Let's see why someone would want that.

This is the solution that runs the tasks concurrently:

var task1 = _client.GetFooAsync();
var task2 = _client.GetBarAsync();

// Both tasks are running.

var v1 = await task1;
var v2 = await task2;

// It doesn't matter if task2 completed before task1:
// at this point both tasks completed and they ran concurrently.

The problem

What about when you don't know how many tasks you'll use?

In this scenario, you can't define the task variables at compile time.
Storing the tasks in a collection, alone, won't solve the problem, since the result of each task was meant to be assigned to a specific variable!

var tasks = new List<Task<string>>();

foreach (var translation in translations)
{
    var translationTask = _client.TranslateAsync(translation.Eng);
    tasks.Add(translationTask);
}

await Task.WhenAll(tasks);

// Now there are N completed tasks, each with a value that
// should be associated to the translation instance that
// was used to generate the async operation.

Solutions

A workaround would be to assign the values based on the index of the task, which of course only works if the tasks were created (and stored) in the same order of the items:

await Task.WhenAll(tasks);

for (int i = 0; i < tasks.Count; i++)
    translations[i].Value = await tasks[i];

A more appropriate solution would be to use Linq and generate a Task that identifies two operations: the fetch of the data and the assignment to its receiver

List<Task> translationTasks = translations
    .Select(async t => t.Value = await _client.TranslateAsync(t.Eng))
    // Enumerating the result of the Select forces the tasks to be created.
    .ToList();

await Task.WhenAll(translationTasks);

// Now all the translations have been fetched and assigned to the right property.

This looks ok, until you need to execute the same pattern on another list, or another single value, then you start to have many List<Task> and Task inside your function that you need to manage:

var translationTasks = translations
    .Select(async t => t.Value = await _client.TranslateAsync(t.Eng))
    .ToList();

var fooTasks = foos
    .Select(async f => f.Value = await _client.GetFooAsync(f.Id))
    .ToList();

var bar = ...;
var barTask = _client.GetBarAsync(bar.Id);

// Now all tasks are running concurrently, some are also assigning the value
// to the right property, but now the "await" part is a bit more cumbersome.

bar.Value = await barTask;
await Task.WhenAll(translationTasks);
await Task.WhenAll(fooTasks);

A cleaner solution (imho)

In this situations, I like to use a helper function that wraps an async operation (any kind of operation), very similar to how the tasks are created with Select above:

async Task Async(Func<Task> asyncDelegate) =>
    await asyncDelegate().ConfigureAwait(false);

Using this function in the previous scenario results in this code:

var tasks = new List<Task>();

foreach (var t in translations)
{
    // The fetch of the value and its assignment are wrapped by the Task.
    var fetchAndAssignTask = Async(async t =>
    {
        t.Value = await _client.TranslateAsync(t.Eng);
    });

    tasks.Add(fetchAndAssignTask);
}

foreach (var f in foos)
    // Short syntax
    tasks.Add(Async(async f => f.Value = await _client.GetFooAsync(f.Id)));

// It works even without enumerables!
var bar = ...;
tasks.Add(Async(async () => bar.Value = await _client.GetBarAsync(bar.Id)));

await Task.WhenAll(tasks);

// Now all the values have been fetched and assigned to their receiver.

Here you can find a full example of using this helper function, which without the comments becomes:

var tasks = new List<Task>();

foreach (var t in translations)
    tasks.Add(Async(async t => t.Value = await _client.TranslateAsync(t.Eng)));

foreach (var f in foos)
    tasks.Add(Async(async f => f.Value = await _client.GetFooAsync(f.Id)));

tasks.Add(Async(async () => bar.Value = await _client.GetBarAsync(bar.Id)));

await Task.WhenAll(tasks);

The AsyncCollector type

This technique can be easily wrapped inside a "Collector" type:

class AsyncCollector
{
    private readonly List<Task> _tasks = new List<Task>();

    public void Register(Func<Task> asyncDelegate) => _tasks.Add(asyncDelegate());

    public Task WhenAll() => Task.WhenAll(_tasks);
}

Here a full implementation and here an usage example.


Note: as pointed out in the comments, there are risks involved when using closures and enumerators, but from C# 5 onwards the use of foreach is safe because closures will close over a fresh copy of the variable each time.

It you still would like to use this type with a previous version of C# and need the safety during closure, the Register method can be changed in order to accept a subject that will be used inside the delegate, avoiding closures.

public void Register<TSubject>(TSubject subject, Func<TSubject, Task> asyncDelegate)
{
    var task = asyncDelegate(subject);
    _tasks.Add(task);
}

The code then becomes:

var collector = new AsyncCollector();
foreach (var translation in translations)
    // Register translation as a subject, and use it inside the delegate as "t".
    collector.Register(translation,
        async t => t.Value = await _client.TranslateAsync(t.Eng));

foreach (var foo in foos)
    collector.Register(foo, async f.Value = await _client.GetFooAsync(f.Id));

collector.Register(bar, async b => b.Value = await _client.GetBarAsync(bar.Id));
await collector.WhenAll();
johnny 5
  • 19,893
  • 50
  • 121
  • 195
Tommaso Bertoni
  • 2,333
  • 1
  • 22
  • 22
  • This is different and last I check somewhat dangerous, You should not be assigning models properties in a closure when you didn't have to – johnny 5 Jul 19 '19 at 19:42
  • can you elaborate on the dangers of this use case? This implementation seems like a fit for the desired experience of having to fetch multiple independent data asynchronously, and then assigning the values to different fields / properties – Tommaso Bertoni Jul 19 '19 at 19:45
  • There are a few issues with doing this, I don't remember them all but I know when you can avoid it you should. Closing over the variable is just one issue you can read about it [here](https://blogs.msdn.microsoft.com/ericlippert/2009/11/12/closing-over-the-loop-variable-considered-harmful/) P.S I wasn't the person who downvoted you I'm just giving you additional information – johnny 5 Jul 19 '19 at 19:51
  • 1
    As you can see at the beginning of the article that you linked: *In C# 5, the loop variable of a foreach will be logically inside the loop, and therefore closures will close over a fresh copy of the variable each time.* Therefore, closures over `foreach` in c# >= 5 are _safe_. – Tommaso Bertoni Jul 19 '19 at 20:01
  • No it means that there is inconsistencies between versions and it just one example of why it's not a recommended practice especially when it could be avoided, to go on to state that it's safe is a fallacy – johnny 5 Jul 20 '19 at 03:25
  • I added support for delegates that don't require closure. – Tommaso Bertoni Jul 20 '19 at 08:41
  • @johnny "You should avoid doing this but I don't remember why" is not a good argument. Unless of course you have reached the status of Jon Skeet or Eric Lippert, in which case even vague memories qualify as good arguments. – Theodor Zoulias Jul 20 '19 at 15:49
  • Don't misquote me, like you misquoted the article. " I don't remember them all but... Closing over the variable is just one issue" 1. Closures lead to bugs when capturing variables in properly. 2. As you mentioned there are inconsistencies between versions. 3. There are more issues with closures. That should be good enough reasons to make assignments inside your funcs. – johnny 5 Jul 20 '19 at 16:06
  • I don't think something that _can_ be dangerous should _never_ been used: i.e. you can always use a temp variable inside the loop if you really want to own the safety measures. – Tommaso Bertoni Jul 22 '19 at 13:17
  • My proposed solution does work and solves the problem with a look & feel like the one described in the answer. With the help of your suggestions @johnny5, the current solution is even better and safer than the first one that I posted, but I'm aware that this implementation is opinionated. It's not that I recommend to use this technique, but I did used it and worked fine, and to me it seemed it resembled what the question asked. – Tommaso Bertoni Jul 22 '19 at 13:18