DotNetAnalyzers / AsyncUsageAnalyzers

Now superseded by Microsoft/vs-threading
https://github.com/Microsoft/vs-threading
Other
121 stars 18 forks source link

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

Open BillWagner opened 9 years ago

BillWagner commented 9 years ago

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 commented 9 years ago

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

tugberkugurlu commented 9 years ago

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 commented 9 years ago

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 commented 6 years ago

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.