CODEWITHSUNDEEP
c#multithreading
bjan
bjan

Reputation: 2030

Duplicate counters are being passed to threads in for loop

This is a sample piece of code for actual problem

//Dictionary to hold unique keys
static Dictionary<int, int> list = new Dictionary<int, int>();

//Worker
static void Do(int index)
{
    list.Add(index, index);
}

static void Main(string[] args)
{
    int counter = 1000;
    for (int i = 0; i < counter; i++)
    {
        int index = i;
        System.Threading.Thread t = new System.Threading.Thread(
                () => Do(index));
        t.Start();
    }
}

Problem: The list does not always contain 1000 elements, because index being passed to Do(index) is duplicating and thread is crashing.

It looks to me that the problem is occuring due to following scenario

  1. index having value 0 is passed to first thread
  2. first thread's Start() is called, but the thread is not actually started yet
  3. on next iteration, index becomes 1
  4. now first thread starts and receives index as 1
  5. second thread is created
  6. second thread's Start() is called and thread receives index as 1
  7. now both threads are trying to add 1 as key
  8. one of them crashes
  9. list's count is short by 1

Is this what happening in this case? What is the solution?

Upvotes: 0

Views: 224

Answers (2)

Daniel Slater
Daniel Slater

Reputation: 4143

As to why exactly this is happening I would guess Matthew Watsons suggestion about not waiting for the last thread to exit is probably correct.

But I would add that you probably don't want to do this by kicking off all your own threads. Parallel.For would handle this for you more efficiently and resolves issues of when everything has completed. e.g. replace your loop above with this.

Parallel.For(0, counter, Do)

Upvotes: 2

Matthew Watson
Matthew Watson

Reputation: 109567

That code shouldn't cause the problem of index being duplicated (since you are already avoiding a closure problem). However, Dictionary<> is NOT threadsafe and therefore the list.Add(index, index) is not safe.

You can solve that issue by locking before you add to the list, like so:

static void Do(int index)
{
    lock (list)
        list.Add(index, index);
}

You also mentioned that the list.Count was not always correct. This is likely to be because you are checking the count before the last thread has completed adding.

You can test that idea by sleeping for a short while before checking the count. (However, sleeping is in general the wrong approach to thread synchronization - but it's OK for a quick test like this. Just don't do it in production code!)

Upvotes: 1

Related Questions