CODEWITHSUNDEEP
c#.netmultithreadinglockingthread-safety
alhazen
alhazen

Reputation: 1915

Basic locking question in C#

The classes:

public class SomeCollection
{
    public void IteratorReset()
    {
      index = -1;
    }

    public bool IteratorNext()
    {
      index++;
      return index < Count;
    }

    public int Count
    {
      get
      {
          return floatCollection.Count;
      }
    }

    public float CurrentValue
    {
      get
      {
        return floatCollection[index];
      }
    }

    public int CurrentIndex
    {
      get
      {
        return intCollection[index];
      }
    }
}

Class that holds reference to 'SomeCollection':

public class ThreadUnsafeClass
{    
    public SomeCollection CollectionObj
    {
       get
       {
           return collectionObj;
        }               
    }      
}

Classes ClassA, ClassB and ClassC contain the following loop that iterates over CollectionObj:

for (threadUnsafeClass.CollectionObj.IteratorReset(); threadUnsafeClass.CollectionObj.IteratorNext(); )
{
    int currentIntIndex = threadUnsafeClass.CollectionObj.CurrentIndex;
    float currentfloatValue = threadUnsafeClass.CollectionObj.CurrentValue;

    // ...    
}

Since I'm only reading CollectionObj in the 3 classes, I'm using multithreading for speedup, but I'm not quite sure how to enforce thread safety. I added a lock in ThreadUnsafeClass when retrieving CollectionObj, but the application throws an out of range exception.

Any help is appreciated.

Thank you !

Upvotes: 0

Views: 231

Answers (3)

Jeremiah Nunn
Jeremiah Nunn

Reputation: 131

Locking the CollectionObj property won't help. One possible problem is that all 3 threads are calling IteratorReset(), which sets the index to -1. Imagine the scenario where A starts the for loop, and gets to the first line in the loop before getting interrupted. Now B comes in and calls IteratorReset(), then gets interrupted to let A run again. Thread A executes the CurrentIndex property, which internally uses index = -1 due to B running. Boom, out of range exception.

There are other ways this can generate bad results, but that's probably the easiest to see. Is the intention to have all three threads go through each item on their own? Or are you expecting A, B, and C to divide up the work (like a consumer queue)?

Upvotes: 1

chezy525
chezy525

Reputation: 4174

The SomeCollection object is being referenced by each of the three classes A,B, and C, each of which is going to try and increment the internal index, causing the error(s). That said, you should be able to read objects in an array from multiple threads with something like the following:

public static object[] sharedList = new object[]{1,2,3,4,5};
public void Worker()
{
   int localSum=0;
   for(int i=0; i<sharedList.length; i++){
      localSum += (int)sharedList[i];
   }
}

The important thing here is that each thread will maintain it's own location within the array, unlike with the collectionObj.

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1503180

You're only reading the CollectionObj property, but then you're mutating the object that the value refers to. See this bit:

for (threadUnsafeClass.CollectionObj.IteratorReset(); 
     threadUnsafeClass.CollectionObj.IteratorNext(); )

Both IteratorReset and IteratorNext mutate SomeCollection by changing the value of index. Basically, you can't do this safely with your current code. Several threads could all call IteratorNext() at the same time, for example. The first call returns true, but before that thread gets a chance to read the values, the other threads make the index invalid.

Why are you using the collection itself for iteration? Typically you'd implement IEnumerable<T> and return a new object in GetEnumerator. That way different threads could each get a different object representing "their" cursor over the same collection. They could all iterate over it, and all see all the values.

Upvotes: 5

Related Questions