current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

I have this Queue declared in my class:

static private Queue<SignerDocument> QUEUE = new Queue<SignerDocument>();

I fill this Queue with some items to process, and I want to process it using multithreading.

Did I need to use lock in this case, to ensure that QUEUE.Dequeue is thread-safe?

static protected void ThreadProcManager()
{     
   lock (typeof(Queue<SignerDocument>))
   {
       if (QUEUE.Count > 0)
       {
           ThreadPool.QueueUserWorkItem(ThreadProc, QUEUE.Dequeue());
        }
    }
}

 static private void ThreadProc(Object stateInfo)
 {
     //Do the work...
 }
share|improve this question
1  
Is there a reason you are using a queue object to buffer work items for ThreadPool.QueueUserWorkItem (which is itself a queue)? The most intuitive behavior would be to perform the ThreadPool.QueueUserWorkItem call immediately where you would currently be doing a QUEUE.Enqueue call. If for some reason you do need this batching behavior, you really need to write a wrapper class for Queue which locks all access calls, such as Count, Peek, Enqueue, Dequeue, etc. – Dan Lyons Mar 28 '12 at 17:33
up vote 4 down vote accepted

Yes, and don't lock on typeof - highly unrecommended. Also you'll need to lock whatever's adding to the QUEUE so that the multithreaded Dequeue() does not race against it.

static protected void ThreadProcManager()
{     
   lock (QUEUE)
   {
       if (QUEUE.Count > 0)
       {
           ThreadPool.QueueUserWorkItem(ThreadProc, QUEUE.Dequeue());
        }
    }
}

 static private void ThreadProc(Object stateInfo)
 {
     //Do the work...
 }
share|improve this answer
up vote 0 down vote

Queue.Dequeue() is not threadsafe, so you need to lock it if it's called from multiple threads.

However, there's not enough information here to say for sure whether a lock is required in this particular case. The call to QUEUE.Dequeue() is not done from a ThreadPool worker, it's done in the ThreadProcManager() function. So the question is really whether you call ThreadProcManager(), or any of the other methods on QUEUE, from multiple threads.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.