2

I've been experimenting lately with async-await and I still cannot get some things to work.

Why does this code not always result with '100' being written to console?

Should not 

await Task.WhenAll(tasks);

wait for all 100 tasks to complete?

static List<int> list = new List<int>(); static void Main(string[] args) { NewMethod(); Console.ReadLine(); } private async static void NewMethod() { var tasks = new List<Task>(); for (int i = 0; i < 100; i++) { tasks.Add(Func(i)); } Console.WriteLine("Lol"); await Task.WhenAll(tasks); Console.WriteLine(list.Count()); } static async Task Func(int i) { await Task.Delay(100); list.Add(i); }

Am I doing something wrong or is this some kind of async-await downside?

Same goes with 

Task.WaitAll(tasks.ToArray());

which I wasn't sure at first if it's equal in this case.

There are a few similar questions but my example is really simple and I can't find explanation in the existing answers.

Improve this question

3 Answers 3

Reset to default
12

The reason you experience the inconsistency in the number of elements is because List<T> isn't thread-safe.

After you await Task.Delay(100), the continuation, which adds elements to the list happens on an arbitrary thread pool, concurrently, because multiple tasks are executing. If you switch you implementation to use ConcurrentBag<int>, this wont happen.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

Try adding a lock(list) before adding the int to the list. You potentially adding from multiple threads to a non thread safe list.

Improve this answer

2 Comments

Lock is not the most performant way. Using either System.Collections.Concurrent.* or a ReaderWriterLock is far more efficient...
Absolutely - Using lock has been chosen only because it makes it easy to reason about the solution and original problem. Performance was never a consideration.
2
  • Do not use list with multi-threaded code - access in unsafe - use one of the collections in System.Collections.Concurrent.
  • Never use 'async void' - it is a horrible thing to support async click handlers in .Net UI applications (http://haacked.com/archive/2014/11/11/async-void-methods/)

The following code will work for your purposes:

using System; using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; namespace ConsoleApplication8 { class Program { static readonly BlockingCollection<int> List = new BlockingCollection<int>(); static void Main(string[] args) { var application = NewMethod(); application.Wait(); Console.ReadLine(); } private async static Task NewMethod() { var tasks = new List<Task>(); for (int i = 0; i < 100; i++) { tasks.Add(Func(i)); } Console.WriteLine("Lol"); await Task.WhenAll(tasks); Console.WriteLine(List.Sum()); } static async Task Func(int i) { await Task.Delay(100); List.Add(i); } } }
Improve this answer

4 Comments

The async void was an effect of quick 'extract method' refactoring but thanks for pointing that out.
Only writing to List in multi-threaded code is a problem, reading should be just fine.
Agreed @Ben - however it's only a matter of time before someone comes along and 'refactors' something. I think it's safer, especially in larger teams, to use concurrent collections when there is any chance of multi-threaded access, as it prevents hard-to-find bugs later down the road.
@BenVoigt If you never write to it, yes. But writing on one thread and reading on multiple is still unsafe. And of course, it's not a contractual behaviour, so it could change in the future

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.