cuncurrency bugs with c# threads and tasks

Question

Description I have a sample code to do threaded parallel online logins. It takes the number of logins to attempt in the Main and passes it to ParallelRun. ParallelRun divides the longinAttemptsCount by the number of threads. It then spawns the threads and passes a thread id and the number of attempts per thread to ThreadAuthenticationTaskRunner

AuthenticateAsync does the actual logging in.

ThreadAuthenticationTaskRunner prints out what thread is starting, and once done, it prints out what thread has ended.

Expected Results

I would expect to see the threads listed in any order but I should not see duplicated Ids.

Actual results

I see some thread Ids missing and others are duplicated. I receive results such as:

What is this concurrency bug I am seeing here?

using System;
using System.Threading.Tasks;
using System.Threading;

using System.Collections.Generic;

namespace Stylelabs.M.WebSdk.Examples
{
    public class Program
    {
        static void Main(string[] args)
        {
            int longinAttemptsCount = 1000;
            Console.WriteLine("Main Thread Started");

            //parallel run 
            ParallelRun(longinAttemptsCount);

            Console.WriteLine("Main Thread Ended");
        }

        /// <summary>
        /// Takes the number of required login attempts and spreads it across threads
        /// </summary>
        /// <param name="longinAttemptsCount"> Number of times to attempt to login</param>
        static void ParallelRun(int longinAttemptsCount)
        {
            int numberOfLoginAttemptsPerThread = 100;

            int numberOfThreads = longinAttemptsCount / numberOfLoginAttemptsPerThread;

            Console.WriteLine("ParallelRun Started: " + numberOfThreads + " threads");

            for (int i = 0; i < numberOfThreads; i++)
            {
                Thread thread1 = new Thread(() => ThreadAuthenticationTaskRunner(i, numberOfLoginAttemptsPerThread));
                thread1.Start();
            }

            Console.WriteLine("ParallelRun Ended: " + numberOfThreads + " threads");
        }

        /// <summary>
        /// Runs parallel logins for each thread
        /// </summary>
        /// <param name="threadId">The identifier of the running thread </param>
        /// <param name="longinAttemptsCount">Number of times to attempt to login </param>
        static async void ThreadAuthenticationTaskRunner(int threadId, int longinAttemptsCount)
        {
            Console.WriteLine("ThreadAuthenticationTaskRunner start for thread: " + threadId);

            string userName = "administrator"; //user to log in

            List<Task<String>> loginAttemptsResultsTasks = new List<Task<String>>();
            //Executing the parallel logins 
            for (int i = 0; i < longinAttemptsCount; i++)
            {
                loginAttemptsResultsTasks.Add(AuthenticateAsync(userName, i, threadId));
            }

            var loginAttemptsResults = await Task.WhenAll(loginAttemptsResultsTasks);

            foreach (string login in loginAttemptsResults)
            {
                Console.WriteLine(login);
            }
            Console.WriteLine("ThreadAuthenticationTaskRunner end for thread: " + threadId);

        }

        /// <summary>
        /// Conducts an asynchronous login on a QA tenant 
        /// </summary>
        /// <param name="userName"> The user to be logged in </param>
        /// <param name="loginId"> The login attempt identifier </param>
        /// <param name="threadId"> The identifier of the running thread </param>
        /// <returns></returns>
        static async Task<String> AuthenticateAsync(String userName, int loginId, int threadId)
        {
            String result;

            try
            {
                //some asynchronous login logic here

                result = "Success: loginId: " + loginId + " threadId: " + threadId;
            }
            catch (Exception e)
            {
                result = "Failure: loginId: " + loginId + " threadId: " + threadId + " error: " + e;
            }

            return result;
        }
    }
}

Seems like you run into the problem that the `int i` is not passed by reference, not by value. Try copying the value to a tmp variable inside the loop: `for (int i = 0; ....) { var copy = i; ..Runner(copy, ...)` — stefan, Apr 21 '20 at 10:12
@stefan hmmm :) that is indeed true. Is this a C# issue or a general concurrency issue? And thank you! — Gakuo, Apr 21 '20 at 10:15
its a detail of the closure you are creating () => ThreadAuthenticationTaskRunner(i, numberOfLoginAttemptsPerThread) - not sure if its specific to c# or its just a general thing with the standard implementation of closures. Wouldn't consider it an issue, just a implementation detail you need to be aware of — Dave, Apr 21 '20 at 10:22
might be useful https://stackoverflow.com/questions/271440/captured-variable-in-a-loop-in-c-sharp — Dave, Apr 21 '20 at 10:23
On a side note, prefer `Task.Run` over `Thread`. Using `async void` with `Thread` can cause some surprising behavior. — Stephen Cleary, Apr 21 '20 at 10:40
@StephenCleary I will look into this further. Just started learning concurrency, I am quite green here :) — Gakuo, Apr 21 '20 at 10:45

score 0 · Accepted Answer · answered Apr 21 '20 at 10:27

The problem here is that

() => ThreadAuthenticationTaskRunner(i, numberOfLoginAttemptsPerThread)

captures a reference to the locale variable i. Then when the function is invoked it will lock up which value the variable currently has. The problem now araises that Thread.Start may return before the lambda was invoked to the loop continues, increases the value for i and then the new thead reads a wrong value for its id.

The simple solution as @stefan alreasy mentioned is to introduce a new variable in the loop, like:

for (int i = 0; i < numberOfThreads; i++)
{
    var tmp = i;
    Thread thread1 = new Thread(() => ThreadAuthenticationTaskRunner(tmp, numberOfLoginAttemptsPerThread));
    thread1.Start();
}

that waw the lambda caputers it's own tmp variable which no one will change. Notice that every loop iteration will have theri own tmp variable and that they will mostlily live on the heap and not the stack.

cuncurrency bugs with c# threads and tasks

1 Answers1