4

.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?

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
Owl
  • 93
  • 1
  • 5
  • 1
    `List` is not thread safe. Either use a thread-safe collection or synchronize access, so that two `Add()` will not run at same time. – Sinatr Jul 02 '21 at 09:46
  • Task.WhenAll starts all tasks at the same time and waits until all are complete. So the order who's first and last is random, i.e. there are race conditions. If the tasks consume the same resources and you want to have control for example on e.g. the order of numbers added to a list, you must not use WhenAll, but await each Task individually in the order you desire. – lidqy Jul 02 '21 at 11:04

3 Answers3

8

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.

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
4

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!

ondrosk3
  • 181
  • 1
  • 4
  • Actually async methods create [promise-style](https://blog.stephencleary.com/2015/04/a-tour-of-task-part-10-promise-tasks.html) tasks, that are less directly associated with a `TaskScheduler` than [delegate-based](https://blog.stephencleary.com/2015/03/a-tour-of-task-part-9-delegate-tasks.html) tasks. But it is indeed possible to wrap the asynchronous `AppendNewIntVal` method in a `Task.Factory.StartNew`, scheduled on a `ConcurrentExclusiveSchedulerPair.ExclusiveScheduler`, and get the desirable synchronization, although it would be an unusual solution to this problem. – Theodor Zoulias Jul 02 '21 at 13:48
3

Yes, race conditions are indeed possible when using async/await in a way that introduces concurrency. To introduce concurrency you must:

  1. Launch multiple asynchronous operations concurrently, i.e. launch the next operation without awaiting the completion of the previous operation, and,
  2. Have no ambient synchronization¹ mechanism in place, namely a SynchronizationContext, that would synchronize the execution of the continuations of the asynchronous operations.

In your case both conditions are met, so the continuations are running on multiple threads concurrently. And since the List<T> class is not thread-safe, you get undefined behavior.

To see what effect a SynchronizationContext has in a situation like this, you can install the Nito.AsyncEx.Context package and do this:

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

        await AppendNewIntVal(appendCount, intVals);

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

FYI many types of applications install automatically a SynchronizationContext when launched (WPF and Windows Forms to name a few). Console applications do not though, hence the need to be extra cautious when writing an async-enabled console application.

¹ It's worth noting that the similar-looking terms synchronized/unsynchronized and synchronous/asynchronous are mostly unrelated. This can be a source of confusion for someone who is not familiar with these terms. The first term is about preventing multiple threads from accessing a shared resource concurrently. The second term is about doing something without blocking a thread.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104