Do race conditions exist when using just async/await?

Question

.NET 5.0

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System.Threading.Tasks;
using System;
using System.Collections.Generic;

namespace AsyncTest
{
    [TestClass]
    public class AsyncTest
    {
        public async Task AppendNewIntVal(List<int> intVals)
        {
            await Task.Delay(new Random().Next(15, 45));

            intVals.Add(new Random().Next());
        }

        public async Task AppendNewIntVal(int count, List<int> intVals)
        {
            var appendNewIntValTasks = new List<Task>();

            for (var a = 0; a < count; a++)
            {
                appendNewIntValTasks.Add(AppendNewIntVal(intVals));
            }

            await Task.WhenAll(appendNewIntValTasks);
        }

        [TestMethod]
        public async Task TestAsyncIntList()
        {
            var appendCount = 30;
            var intVals = new List<int>();

            await AppendNewIntVal(appendCount, intVals);

            Assert.AreEqual(appendCount, intVals.Count);
        }
    }
}

The above code compiles and runs, but the test fails with output similar to:

Assert.AreEqual failed. Expected:<30>. Actual:<17>.

In the above example the "Actual" value is 17, but it varies between executions.

I know I am missing some understanding around how asynchronous programming works in .NET as I'm not getting the expected output.

From my understanding, the AppendNewIntVal method kicks off N number of tasks, then waits for them all to complete. If they have all completed, I'd expect they would have each appended a single value to the list but that's not the case. It looks like there's a race condition but I didn't think that was possible because the code is not multithreaded. What am I missing?

Answer 1

Yes, if you don't await each awaitable immediately, i.e. here:

appendNewIntValTasks.Add(AppendNewIntVal(intVals));

This line is in async terms equivalent to (in thread-based code) Thread.Start, and we now have no safety around the inner async code:

intVals.Add(new Random().Next());

which can now fail in the same concurrency ways when two flows call Add at the same time. You should also probably avoid new Random(), as that isn't necessarily random (it is time based on many framework versions, and can end up with two flows getting the same seed).

So: the code as shown is indeed dangerous.

The obviously safe version is:

        public async Task AppendNewIntVal(int count, List<int> intVals)
        {
            for (var a = 0; a < count; a++)
            {
                await AppendNewIntVal(intVals);
            }
        }

It is possible to defer the await, but you're explicitly opting into concurrency when you do that, and your code needs to handle it suitably defensively.

Answer 2

Yes, race conditions do exist.

Async methods are basically tasks that can potentially run in parallel, depending on a task scheduler they are submitted to. The default one is ThreadPoolTaskScheduler, which is a wrapper around ThreadPool. Thus, if you submit your tasks to a scheduler (thread pool) that can execute multiple tasks in parallel, you are likely going to run into race conditions.

You could make your code a bit safer:

lock (intVals) intVals.Add(new Random().Next());

But then this opens up another can of worms :)

If you are interested in more details about async programming, see this link. Also this article is quite useful and explains best practices in asynchronous programming.

Happy (asynchronous) coding!