Skip to content
This repository has been archived by the owner on Nov 8, 2018. It is now read-only.

Use Task.WhenAll or Task.WhenAny instead of awaiting in loops #6

Open
BillWagner opened this issue Nov 19, 2014 · 4 comments
Open

Use Task.WhenAll or Task.WhenAny instead of awaiting in loops #6

BillWagner opened this issue Nov 19, 2014 · 4 comments

Comments

@BillWagner
Copy link

Again, I'm not sure exactly how to spot this, but too much code does something like:

for (int i = 0; i < 100; i++)
    await SomeTask(i);

Instead of:

var tasks = from i in Enumerable.Range(0,100)
            select SomeTask(i);
await tasks.WhenAll();
@sharwell
Copy link
Member

Interesting. Probably a "run loop in parallel" refactoring.

@tugberkugurlu
Copy link

This is tricky as it may lead user to write buggy code because it's close to impossible to know whether the executing code is thread safe or not. E.g.:

public class HomeController : Controller
{
    private readonly MyDbContext _context;

    public HomeController(MyDbContext context)
    {
        _context = context;
    }

    public Task<ActionResult> Index()
    {
        var parameters = new[] { "cheap", "expensive", "normal" };
        foreach(var parameter in parameters)
        {
            var results = await _context.Items.FirstOrDefaultAsync(x => x.Category == parameter);
            // ...
        }

        // ...
    }
}

This code is safe but if we turn this into Task.WhenAll, you will get weird concurrency exceptions as EF's DbContext is not thread safe. So, suggesting this code fix may confuse the developer.

@ChrSteinert
Copy link

I'd say awaiting in loops is a rather valid scenario like the one above. Awaiting in loops does not have to be a faulty approach to parallelism rather than offloading to background threads. EF is a perfect example for this.
So detecting valid cases for this rule seems super hard to me.

@AArnott
Copy link

AArnott commented May 2, 2018

I think the only case we might want to flag users to avoid is:

Task[] taskArray;
for (int i = 0; i < taskArray.Length; i++)
    await taskArray[i];

Here, changing to Task.WhenAll(taskArray) is a relatively safe change. It's not truly equivalent, since the looping code will fail immediately when a task it's awaiting on faults, whereas WhenAll will wait for all tasks to complete before returning.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants