Why collection doesn't fill properly when I am using tasks?

Question

I have pretty simple piece of code:

public class CustomTester
{
    public async Task RunTheTestAsync()
    {
        IList<int> someCollectionToFill = new List<int>();

        var someCollectionToIterate = Enumerable.Range(0, 20);

        IEnumerable<Task> tasks = someCollectionToIterate.Select(
            valueFromCollection => Task.Run(async () =>
        {
            try
            {
                await Task.Delay(1);

                someCollectionToFill.Add(valueFromCollection);
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
        })).ToList();

        await Task.WhenAll(tasks);

        PrintCollectionValues(someCollectionToFill);
    }

    private void PrintCollectionValues(IList<int> collection)
    {
        Console.WriteLine("Total count: " + collection.Count);
    }
}

I am executing it like this:

await new CustomTester().RunTheTestAsync();

When the code executed, I have following output:

Total count: 13

or:

Total count: 14

etc...

But I am expecting:

Total count: 20

Why is this happening?

Any particular reason you expect non-thread safe collection to safely work in threaded scenario? — Alexei Levenkov, Mar 04 '21 at 07:48
@AlexeiLevenkov There is no reason, actually. Do I have to use `lock`? — Phil, Mar 04 '21 at 07:52
This about the time you research thread safety, yes a lock would help here. But you are flying blind unless you understand why it will help — TheGeneral, Mar 04 '21 at 07:53
@TheGeneral I understand that `someCollectionToFill` shared resource between the different tasks, so I need to `lock` it during filling. But I don't really understand why the collection fills not completely without using `lock` and why I don't have any exceptions here. Do you have any notes or maybe something to read about this topic? :) — Phil, Mar 04 '21 at 07:57
You will have to use lock yourself to create a "thread safe list" but fortunately there is already some concurrent collection. You can use ConcurrentBag for your "someCollectionToFill" and everything will be fine. — Arcord, Mar 04 '21 at 07:57
A list is based on an non thread safe array, which in turn is just a bunch of memory. Every time you add to a list, it has the potential to resize that base array. Since you cannot actually resize an array, the runtime actually needs to make a new bigger array, and copy the current contents. You can image if 2 threads are trying to insert and copy at the same time things are going to get fun. Its like multiple people trying to put things in 1 bucket, the bucket gets full then you have to get a bigger bucket and tip it in. What happens when one person is putting stuff, and another is tipping — TheGeneral, Mar 04 '21 at 08:00
@Phil If you use something which is not thread safe in multiple threads, it doesn't mean automatically that an exception is thrown. It is also possible that the object is not in the expected state, which is what you observed. — SomeBody, Mar 04 '21 at 08:01
@Arcord the `ConcurrentBag` [is not](https://stackoverflow.com/questions/15400133/when-to-use-blockingcollection-and-when-concurrentbag-instead-of-listt/64823123#64823123) a thread-safe `List`. It is a collection that has an `Add` method and a `Count` property, and that's where the similarities end. For example you can't remove a specific item from a `ConcurrentBag`. I wish that its `Add` method had been given a different name (`Store`, `Put`, `Keep` or something else), so that people didn't associate it with a `List` so frequently. — Theodor Zoulias, Mar 04 '21 at 10:15
I agree it's not the same. I just assumed (wrongly?) he only needed to be able to "Add" and the order was not important. — Arcord, Mar 04 '21 at 10:31

score 4 · Accepted Answer · edited Mar 04 '21 at 10:19

You are using a list, a non threadsafe collection, concurrently from multiple threads. This is not safe. Either use

lock(someCollectionToFill){
    someCollectionToFill.Add(valueFromCollection);
}

or

var someCollectionToFill = new ConcurrentBag<int>();

I would highly recommend reading up on threadsafety before trying to use any multithreading. Multi threading best practices is a good start, but it does not cover everything, so I would recommend some googling around. Writing multithreaded code is difficult, and it is very easy to make mistakes that cause intermittent bugs that are very difficult to debug and understand.

Thank you. I will definitely get acquainted with the topic. – Phil Mar 04 '21 at 08:06 — Phil, Mar 04 '21 at 08:06

Why collection doesn't fill properly when I am using tasks?

1 Answers1