The correct way to await inside a foreach loop

Question

Is this the correct way to use foreach loops whist using async ? Is there a better more efficient way? IAsyncEnumerable? (Ignore the fact that the two tables could be joined together this is a simple example)

public async Task<IList<ContactAndAccounts>> GetAll()
{
    var accounts = await _dbContext.Account.Where(x => x.Name == "Amazing").ToListAsync();

    foreach(var account in accounts)
    {
           accounts.Contacts = await GetContact(account.Id);
    }

    return accounts;
}

public async Task<IList<contact>> GetContact(Guid id)
{
    return await _dbContext.Contact.Where(x => x.AccountLinkId = id).ToListAsync();
}

You shouldn't get the contacts in separate queries anyway. Why don't you `Include` them? — Gert Arnold, Jan 30 '20 at 11:14
The problem here has nothing to do with await and loops. It's the missing relation. The fix is to use `Include(acc=>acc.Contacts)`. `Account` should have a `Contacts` property and the DbContext should contain the 1-to-many relation — Panagiotis Kanavos, Jan 30 '20 at 11:23
With a correct dbcontext, the query becomes just `_dbContext.Account.Include(acc=>acc.Contatcts).Where(...).ToListAsync()`. No looping is needed — Panagiotis Kanavos, Jan 30 '20 at 11:25
@PanagiotisKanavos i agree but please read the original questions "Ignore the fact that the two tables could be joined together this is a simple example" — Rhodes73, Jan 30 '20 at 11:32
That's the main problem, it can't be ignored. Everything else is band-aids. This won't solve the real problem. The correct fix is *easier* than every one of the solutions posted here — Panagiotis Kanavos, Jan 30 '20 at 11:35
@Rhodes73 the only answer that mitigates the problem is Michael Hancock's - at least load all contacts in a single query instead of running 100 separate queries. — Panagiotis Kanavos, Jan 30 '20 at 11:37

Michael Hancock · Accepted Answer · 2020-01-30T13:11:29.487

4

I agree with Johnathan Barclay's answer but would say that from a database perspective you might find it is faster to do a single query big query than lots of small queries.

Doing one query and passing in all of your ids is usually less expensive than multiple seperate queries.

public async Task<IList<ContactAndAccounts>> GetAll()
{
    var accounts = await _dbContext.Account.Where(x => x.Name == "Amazing").ToListAsync();

    var contacts = await GetContacts(accounts.Select(o => o.Id));

    // Map contacts to accounts in memory

    return accounts;
}

public async Task<IList<contact>> GetContacts(List<Guid> ids)
{
    return await _dbContext.Contact.Where(x => ids.Contain(x.AccountLinkId)).ToListAsync();
}

edited Jan 30 '20 at 13:11

answered Jan 30 '20 at 11:13

Michael Hancock

2,673
1
18
37

1

Yes, I was focusing on the `foreach` loop and didn't notice this. +1 – Johnathan Barclay Jan 30 '20 at 11:19
The *real* solution would be to add a relation between `Account` and `Contact` and use `Include(acc=>acc.Contacts)` – Panagiotis Kanavos Jan 30 '20 at 11:19
@PanagiotisKanavos Short of this OP could do a join if no naviagtion property exists. But I always find they can end up looking rather messy in Linq. – Michael Hancock Jan 30 '20 at 11:20
Which is why the relation must be added. Everything else is band-aids. The code in all answers is already a lot more complicated than a correct LINQ query would be – Panagiotis Kanavos Jan 30 '20 at 11:21
If you take a look at this [GitHub thread](https://github.com/dotnet/efcore/issues/17068), you will see that currently there is a huge difference in what is supported for collection navigation properties and `GroupJoin`s (although technically they should be equivalent). Which makes usage of navigation properties almost mandatory. – Ivan Stoev Jan 30 '20 at 23:24
That is strange, you would think the generated query would be effectively the same! – Michael Hancock Jan 31 '20 at 08:16

Johnathan Barclay · Answer 2 · 2020-01-30T11:12:12.020

0

It's definitely not the most efficient way.

Each await causes the loop to pause until the Task is complete.

You want to allow all the tasks to run simultaneously:

public async Task<IList<ContactAndAccounts>> GetAll()
{
    var accounts = await _dbContext.Account.Where(x => x.Name == "Amazing").ToListAsync();

    await Task.WhenAll(accounts.Select(async account => 
    {
        accounts.Contact = await GetContact(account.Id);
    }));

    return accounts;
}

Select will yield a Task for each item, which can be awaited through Task.WhenAll.

edited Jan 30 '20 at 11:12

answered Jan 30 '20 at 11:10

Johnathan Barclay

18,599
1
22
35

Yes WhenAll is the way to go. Congrats you were faster than I :) – aloisdg Jan 30 '20 at 11:11
1

This still executes 101 queries when only 1 is needed. Running the 100 queries in parallel won't improve performance, it will increase lock contention – Panagiotis Kanavos Jan 30 '20 at 11:20
@PanagiotisKanavos yes, I notice that now, but I answered the question at hand. Michael has elaborated on that already in his answer. – Johnathan Barclay Jan 30 '20 at 11:22

score -2 · Answer 3 · answered Jan 30 '20 at 11:16

-2

Is there a better more efficient way? IAsyncEnumerable?

Yes! Change signature and replace accounts.Contacts = await GetContact(account.Id); with yield return await GetContract(account.Id).

answered Jan 30 '20 at 11:16

Alexbogs

380
4
9

2

That's not more efficient. The fundamental problem is that the related objects are loaded one by one using different queries instead of a single query that loads all of them at once. IAsyncEnumerable won't fix that – Panagiotis Kanavos Jan 30 '20 at 11:17

The correct way to await inside a foreach loop

3 Answers3