CODEWITHSUNDEEP
c#.netmultithreadingc#-4.0race-condition
Jon
Jon

Reputation: 40032

Race Condition on Queue

The below code handles a socket message coming in with a 2 parameters. It puts information into a queue and is processed on another thread. My question is if 2 messages come in right after the other and are then dequeued and sent to the method ProcessData, is there a race condition on ProcessData?

private void DataIn(long Code, string Message)
{
  if (!Started)
  {
    if (DataInQueue == null)
      DataInQueue = new Queue();
    DataInThread = new Thread(new ThreadStart(ThreadProcedure));
    DataInThreadEnding = false;
    DataInThread.IsBackground = true;
    DataInThread.Start();
    Started = true;
  }
  DataInQueue.Enqueue(new cDataIn(Code, Message));
}

private void ThreadProcedure()
{
   while (!ProgramEnding)
   {
     Queue mySyncdQ = Queue.Synchronized(DataInQueue);
     if (mySyncdQ != null && mySyncdQ.Count > 0)
     {
        cDataIn data = null;
        // Creates a synchronized wrapper around the Queue. 
        if (mySyncdQ.Count > 0)
          data = (cDataIn)mySyncdQ.Dequeue();

        ProcessData(data);
    }
  }

}

Upvotes: 1

Views: 2376

Answers (3)

gjvdkamp
gjvdkamp

Reputation: 10516

Yes I think this can even leave your Queue in an inconstistant state. You should use the Concurrent Queue BlockingCollection that ships with .Net 4. It's threadsafe out of the box and optimzed.

Regards Gert-Jan

Upvotes: 4

Yahia
Yahia

Reputation: 70369

UPDATE

Queue is not used in a thread-safe manner in your code... the code you show is not enough to be sure whether there is a race condition but with the ConcurrentQueue you get a better performance... and in the ThreadProcedure you can have calling ProcessData with null and as far as I can tell to be on the safe side ProcessData should be reentrant for all this to work...

Use a ConcurrentQueue - that avoids some possible problems... and check out BlockingCollection which is designed for thread-safe Producer/Consumer scenarios... both work mostly lock-free and are really fast...

Upvotes: 5

svick
svick

Reputation: 244837

Your code is not thread safe. Notice this in the documentation for Queue.Synchronized():

To guarantee the thread safety of the Queue, all operations must be done through this wrapper only.

You're using the queue directly, so the code is not thread-safe. To fix this, always use just the returned wrapper, as the documentation says.

Or, if you're under .Net 4, use ConcurrentQueue<T>.

If you wanted to use generic queue on an previous version of .Net, you would could use Queue<T> and always access it in a lock.

Upvotes: 3

Related Questions