Parallelize / Multi-Thread a singleton object method call

Question

Code Details:

// Singleton class CollectionObject

public class CollectionObject
{
    private static CollectionObject instance = null;        

// GetInstance() is not called from multiple threads
    public static CollectionObject GetInstance()
    {
        if (CollectionObject.instance == null)
            CollectionObject.instance = new CollectionObject();

        return CollectionObject.instance;
    }

// Dictionary object contains Service ID (int) as key and Service object as the value
// Dictionary is filled up during initiation, before the method call ReadServiceMatrix detailed underneath

    public Dictionary<int, Service> serviceCollectionDictionary = new Dictionary<int,Service>();

    public Service GetServiceByIDFromDictionary(int servID)
    {
        if (this.serviceCollectionDictionary.ContainsKey(servID))
            return this.serviceCollectionDictionary[servID];
        else
            return null;
    }

}

 DataTable serviceMatrix = new DataTable();

 // Fill serviceMatrix data table from the database

 private int ReadServiceMatrix()
 {
    // Access the Singleton class object
    CollectionObject collectionObject = CollectionObject.GetInstance();

    // Parallel processing of the data table rows
    Parallel.ForEach<DataRow>(serviceMatrix.AsEnumerable(), row =>
    {
       //Access Service ID from the Data table
       string servIDStr = row["ServID"].ToString().Trim();

       // Access other column details for each row of the data table

       string currLocIDStr = row["CurrLocId"].ToString().Trim();
       string CurrLocLoadFlagStr = row["CurrLocLoadFlag"].ToString().Trim();
       string nextLocIDStr = row["NextLocId"].ToString().Trim();
       string nextLocBreakFlagStr = row["NextLocBreakFlag"].ToString().Trim();
       string seqStr = row["Seq"].ToString().Trim();

       int servID = Int32.Parse(servIDStr);
       int currLocID = Int32.Parse(currLocIDStr);
       int nextLocID = Int32.Parse(nextLocIDStr);
       bool nextLocBreakFlag = Int32.Parse(nextLocBreakFlagStr) > 0 ? true : false;
       bool currLocBreakFlag = Int32.Parse(CurrLocLoadFlagStr) > 0 ? true : false;
       int seq = Int32.Parse(seqStr);

       // Method call leading to the issue (definition in Collection Object class)
       // Fetch service object using the Service ID from the DB                      

       Service service = collectionObject.GetServiceByIDFromDictionary(servID);

       // Call a Service class method

       service.InitLanes.Add(new Service.LaneNode(currLoc.SequentialID, currLocBreakFlag, nextLoc.SequentialID, nextLocBreakFlag, seq));

    }

Issue that happens is:

• In the code above for all the Service objects in the dictionary, the subsequent method call is not made, leading to issues in further processing. It has to o with fetching the Service object from the dictionary in parallel mode

• The db an dictionary contains all the Ids /Service objects, but my understanding is when processing in Parallel mode for the Singleton class, few of the objects are skipped leading to the issue.

• In my understanding the service id passed and service object created is local to a thread, so there shouldn't be an issue that I am facing. This kind of issue is only possible, when for a given method call one thread replace service id value of another thread by its, thus both end up with Service object and few are thus skipped, which is strange in my view until and unless I do not understand the Multi threading in this case correctly

• Currently I am able to run the same code in non threaded mode by using the foreach loop instead of Parallel.ForEach / Parallel.Invoke

Please review and let me know your view or any pointer that can help me resolve the issue

Answer 1

1.Implementing singleton always think about using of it in mulithreaded way. Always use multithreaded singleton pattern variant, one of them - lazy singleton. Use Lazy singleton using System.Lazy with appropriate LazyThreadSafeMode consturctor argument:

public class LazySingleton3
{
     // static holder for instance, need to use lambda to enter code here
     //construct since constructor private
     private static readonly Lazy<LazySingleton3> _instance
         = new Lazy<LazySingleton3>(() => new LazySingleton3(), 
                                          LazyThreadSafeMode.PublicationOnly);

     // private to prevent direct instantiation.
     private LazySingleton3()
     {
     }

     // accessor for instance
     public static LazySingleton3 Instance
     {
         get
         {
             return _instance.Value;
         }
     }
}

Read about it here

2.Use lock-ing of your service variable in parallel loop body

// Method call leading to the issue (definition in Collection Object class)
// Fetch service object using the Service ID from the DB                      
Service service = collectionObject.GetServiceByIDFromDictionary(servID);

lock (service)
{    
    // Call a Service class method        
    service.InitLanes.Add(new Service.LaneNode(currLoc.SequentialID,
                          currLocBreakFlag, nextLoc.SequentialID,
                          nextLocBreakFlag, seq));
}

3.Consider to use multithreading here. Using lock-ing code make your code not so perfomant as synchronous. So make sure you multithreaded/paralelised code gives you advantages

4.Use appropriate concurrent collections instead of reinventing wheel - System.Collections.Concurrent Namespace

Answer 2

In my understanding the service id passed and service object created is local to a thread

Your understanding is incorrect, if two threads request the same service id the two threads will be both working on the same singular object. If you wanted separate objects you would need to put some kind of new Service() call in GetServiceByIDFromDictionary instead of a dictionary of existing values.

Because multiple threads could be using the same service objects I think your problem lies from the fact that service.InitLanes.Add is likely not thread safe.

The easiest fix is to just lock on that single step

  //...SNIP...

  Service service = collectionObject.GetServiceByIDFromDictionary(servID);

  // Call a Service class method, only let one thread do it for this specific service instance, 
  // other threads locking on other instances will not block, only other threads using the same instance will block
  lock(service)
  {
      service.InitLanes.Add(new Service.LaneNode(currLoc.SequentialID, currLocBreakFlag, nextLoc.SequentialID, nextLocBreakFlag, seq));
  }

}

This assumes that this Parallel.Foreach is the only location collectionObject.GetServiceByIDFromDictionary is used concurrently. If it is not, any other locations that could potentially be calling any methods on returned services must also lock on service.

However if Service is under your control and you can somehow modify service.InitLanes.Add to be thread safe (perhaps change InitLanes out with a thread safe collection from the System.Collections.Concurrent namespace) that would be a better solution than locking.