Multi threading not executing properly

Question

I have a function here that calls another function in multiple threads. In the second functions, the value of each element of refValuesd[,] is 1. But when I check the elements of the same 2D array in the graph1() function after the multiple threads are called, I get different values for the elements of refValuesd[,].I am a novice in multithreading.

void graph1()
    {
        for (int j = 0; j < 366; j++) //loop to refresh element values
        {
            refValues[j] = 0;
            threshValues[j] = 0;
            y_Values[j] = 0;
            y__Values[j] = 0;
            yValues[j] = 0;
            for (int k = 0; k < 1000; k++)
            {
                threshValuesd[k,j] = 0;
                refValuesd[k,j] = 0;
                y__Valuesd[ k,j] = 0;
                y_Valuesd[k,j] = 0;
                yValuesd[k,j] = 0;
            }

        }

        List<string>[] list = new List<string>[4];//manpower details
        list = A.mandetselect();
        int number = A.Countmandet();//retuns an integer value
        string[] trade = list[1].ToArray();
        string[] license = list[2].ToArray();
        string[] training = list[3].ToArray();
        string[] display_status = list[4].ToArray();

        List<string>[] listc = new List<string>[14];//Project details
        listc = A.Select();
        int numberc = A.Count();

        string abc = "";
        int q = 0;
        for (int j = 0; j < number; j++)
        {

            if (!display_status[j].Equals("NO") && (selection == "ALL" || (selection == "ALL-LAE" && license[j] != "") || (selection == "ALL-NON LAE" && license[j] == "") || (selection == "AVIONICS -ALL" && trade[j] == "Avionics") || (selection == "AVIONICS-NON LAE" && trade[j] == "Avionics" && license[j] == "") || (selection == "AVIONICS-LAE" && trade[j] == "Avionics" && license[j] != "") || (selection == "AIRFRAME-ALL" && trade[j] == "Airframes") || (selection == "AIRFRAME-NON LAE" && trade[j] == "Airframes" && license[j] == "") || (selection == "AIRFRAME-LAE" && trade[j] == "Airframes" && license[j] != "")))
            {
                int z = numberc;
                string[] nameofproj = listc[0].ToArray();
                int copy = q;
                int copy2 = j;
                string a = abc;
                string[] name = list[0].ToArray();
                List<string>[] lista = new List<string>[5];
                string[] status = listc[13].ToArray();
                thread[copy] = new Thread(() => graph1threader(a, name[copy2], lista, z, nameofproj, status, copy));
                thread[copy].Start();

                q++;

            }
        }


        for (int j = 0; j < 366; j++)
        {
            for (int k = 0; k < q; k++)
            {
                threshValues[j] += threshValuesd[k, j];                   
                refValues[j] += refValuesd[k, j];
                y__Values[j] += y__Valuesd[k, j];
                y_Values[j] += y_Valuesd[k, j];
                yValues[j] += yValuesd[k, j];
            }
        }


            for (int j = 0; j < 366; j++)
            {
                DateTime temp = G.AddDays(j);
                string temper = temp.ToShortDateString();
                y__Values[j] = y__Values[j] - y_Values[j];
                xNames[j] = temper;
            }

        chart1.Series[1].Points.DataBindXY(xNames, y_Values);           


        chart1.Series[2].Points.DataBindXY(xNames, y__Values);


        chart1.Series[3].Points.DataBindXY(xNames, refValues);


        chart1.Series[4].Points.DataBindXY(xNames, threshValues);


        chart1.Series[5].Points.DataBindXY(xNames, yValues);



    }

Here is the function that is executed on multiple threads:

void graph1threader(string abc,string nameofj,List<string>[] lista,int numberc,string[] nameofproj,string[] status,int copy )
    {
        DBConnect A = new DBConnect();
        int x = copy;
        string[] projname;
        string[] country;
        string[] start;
        string[] end;
        abc = nameofj.Replace(" ", "_");
        lista = A.manprojselect(abc);
        projname = lista[0].ToArray();
        country = lista[2].ToArray();
        start = lista[3].ToArray();
        end = lista[4].ToArray();

        for (int k = 0; k < 366; k++)//basic
        {

            refValuesd[x, k]++;
             refValuesd[copy,k].ToString());
            threshValuesd[x, k] = 0.8;
            string Status = "";
            int flag = 0;

            for (int l = 0; l < A.Countproj(abc); l++)
            {

                for (int m = 0; m < numberc; m++)
                {
                    if (nameofproj[m] == projname[l])
                    {
                        Status = status[m];
                    }
                }


                DateTime shuru = DateTime.ParseExact(start[l],
                               "dd-MM-yyyy hh:mm:ss",
                               CultureInfo.InvariantCulture);
                DateTime anth = DateTime.ParseExact(end[l],
                               "dd-MM-yyyy hh:mm:ss",
                               CultureInfo.InvariantCulture);
                if (temp >= shuru && temp <= anth)
                {
                    if (Status != "PLANNED" && Status != "LO" && flag == 0)
                    {
                        y_Valuesd[x,k]++;//BASIC UTILISATION
                        flag = 1;
                    }
                    if (Status == "IP" || Status == "OTD")
                        y__Valuesd[x,k]++;//EXCESS
                    if (Status == "PLANNED")
                    {
                        yValuesd[x,k]++;//UNUTILISED

                    }

                }

            }


        }

    }

Answer 1

Make sure you surround access to refvalued with lock sections and you will start in the right direction. You have to design your locking not to create race conditions and deadlocks as well.

EDIT: So, having reviewed your code, here are a few comments

It looks like you kick off the graph function, which in turn executes the graph1threader function on multiple threads. I won't question whether or not that is necessary - the assumption is that you've already decided it is.

Order of events

It looks like you don't stop to wait for all the graph1threader threads to complete before continuing in your second loop. So here is a question:

• Do you want the graph1threader jobs to complete before you continue?

If so, have you looked at Task? Instead of creating threads, you could almost literally swap Thread with Task and then, once you have completed the creation of all your Task objects and started them, you could put Task.WaitAll after your for (int j = 0; j < number; j++) loop to wait for them to complete before you then do the third for loop in graph1:

        thread[copy] = new Task(() => graph1threader(a, name[copy2], lista, z, nameofproj, status, copy));
        thread[copy].Start();
        q++;
    }
}
Task.WaitAll(thread);

for (int j = 0; j < 366; j++)
{
    for (int k = 0; k < q; k++)
    {
        threshValues[j] += threshValuesd[k, j];
        refValues[j] += refValuesd[k, j];
        y__Values[j] += y__Valuesd[k, j];
        y_Values[j] += y_Valuesd[k, j];
        yValues[j] += yValuesd[k, j];
    }
}

If you don't want to use Task (or can't) Thread.Join will also work, but Task supports cancellation easier than Thread so if you have a UI with long running operations it will make things easier for you.

Shared fields

The following variables are used by both functions (please ignore any incorrect variables types, it is the names that are important):

double[,] threshValuesd;
int[,] refValuesd;
int[,] y__Valuesd;
int[,] y_Valuesd;
int[,] yValuesd;

I am calling this list bookmark A to be used later

All these potentially need protecting against multi-threaded race conditions etc.

How to protect the shared fields

Regardless of whether you wait or not, you need to protect your shared fields in graph1threader. So, if I've read your code properly:

• you pass a unique, incremented value to graph1threader (using q++ in your 2nd for loop in graph1)
• Therefore the following lines will not compete to set values between threads in graph1threader:
  • refValuesd[x, k]++;
  • threshValuesd[x, k] = 0.8;
  • y_Valuesd[x,k]++;
  • y__Valuesd[x,k]++;
  • yValuesd[x,k]++;

(Side note: these variable names are incomprehensible to me, you might try naming them in a more descriptive way than yValuesd, y_Valuesd and y__Valuesd to help you debug this later).

However, even if threads don't compete to update values in the same array slot, you probably have problems coming in the form of memory barriers and read/write access to a single array slot. Therefore, I would recommend doing this, quite simply declare a class field:

private readonly object SyncRoot = new object();

and then around all access to any of the shared fields I mentioned above bookmark A you need to use (picking your first loop as an example):

lock (this.SyncRoot) {
    for (int j = 0; j < 366; j++)
    {
        for (int k = 0; k < q; k++)
        {
            threshValues[j] += threshValuesd[k, j];
            refValues[j] += refValuesd[k, j];
            y__Values[j] += y__Valuesd[k, j];
            y_Values[j] += y_Valuesd[k, j];
            yValues[j] += yValuesd[k, j];
        }
    }
}

Keep lock calls as infrequent as possible, but as close to the shared resource as possible. By that I mean that you could lock inside the inner for loop if you wanted to, but that will be slower, however you may need that to allow other threads to proceed more frequently if they also lock the same object.

NOTE: This technique of using a shared lock assumed you wanted the graph1threader threads to be running at the same time as your third for loop in graph1 (i.e. my comment about Task objects wasn't required). If this is not the case, I think you could create a local object within each function and lock on that instead. Each thread would therefore have a different lock object. Because no thread accesses the same slot in your arrays at the same time, this would just enforce memory barriers and ensure that all threads saw the same values when they read them.

Sorry this was so long, it was hard to know where to start without know more about the assumptions you went through to build this code.

Answer 2

There's probably a few problems in there, but two I can spot:

Firstly, the threads are trying to do refValuesd[x, k]++ which isn't threadsafe.

Try this instead:

Interlocked.Increment(ref refValuesd[x, k]);

Secondly, you are not waiting for all the threads to terminate before using the data they've generated. Try adding this just before the for (int j = 0; j < 366; j++) line:

foreach (var thread in threads)
    thread.Join();

It looks like you have much to learn, so I recommend that you read this free ebook:

http://www.albahari.com/threading/