22

I have not used concurrent queue before.

Is it OK to use TryDequeue as below, in a while loop? Could this not get stuck forever?

var cq = new ConcurrentQueue<string>();
cq.Enqueue("test");

string retValue;

while(!cq.TryDequeue(out retValue))
{
    // Maybe sleep?
}

//Do rest of code
ManInMoon
  • 6,795
  • 15
  • 70
  • 133
  • 2
    Don't you mean while(!cq.TryDequeue(out retValue)) ? – Dennis_E May 23 '14 at 14:11
  • @ManInMoon are you talking about through multiple threads? My assumption would be that a queue designed specifically for concurrency in the .NET library would be safe to use in a while loop like this. Obviously through your testing you've determined that in single thread usage it's totally safe. – Ryanman May 23 '14 at 14:14
  • Your question suggests that another thread will be producing the queue items and if that's the case you should be using a BlockingCollection instead. Here's a simple example: https://gist.github.com/ronnieoverby/1f1750e67ca3c5b7af15b9abc31fd331 – Ronnie Overby Mar 20 '19 at 17:14

2 Answers2

21

It's safe in the sense that the loop won't actually end until there is an item it has pulled out, and that it will eventually end if the queue has an item to be taken out. If the queue is emptied by another thread and no more items are added then of course the loop will not end.

Beyond all of that, what you have is a busy loop. This should virtually always be avoided. Either you end up constantly polling the queue asking for more items, wasting CPU time and effort tin the process, or you end up sleeping and therefore not actually using the item in the queue as soon as it is added (and even then, still wasting some time/effort on context switches just to poll the queue).

What you should be doing instead, if you find yourself in the position of wanting to "wait until there is an item for me to take" is use a BlockingCollection. It is specifically designed to wrap various types of concurrent collections and block until there is an item available to take. It allows you to change your code to queue.Take() and have it be easier to write, semantically stating what you're doing, be clearly correct, noticeably more effective, and completely safe.

svick
  • 236,525
  • 50
  • 385
  • 514
Servy
  • 202,030
  • 26
  • 332
  • 449
17

Yes it is safe as per the documentation, but it is not a recommended design.

It might get "Stuck forever" if the queue was empty at the first call TryDequeue, and if no other thread pushes data in the queue after that point (you could break the while after N attempts or after a timeout, though).

ConcurrentQueue offers an IsEmpty member to check if there are items in the Queue. It is much more efficient to check it than to loop over a TryDequeue call (particularly if the queue is generally empty)

What you might want to do is :

while(cq.IsEmpty())
{
    // Maybe sleep / wait / ...
}

if(cq.TryDequeue(out retValue))
{
...
}

EDIT: If this last call returns false: another of your threads dequeued the item. If you don't have other threads, this is safe, if you do, you should use while(TryDequeue)

quantdev
  • 23,517
  • 5
  • 55
  • 88
  • Ok thank you - that makes sense. BUT what are you supposed todo if the if(cq.TryDequeue(out retValue)) is false? – ManInMoon May 23 '14 at 14:37
  • That's kinda my concern. Will the while() in your EDIT get stuck? – ManInMoon May 23 '14 at 14:56
  • `cq.IsEmpty` is a property, not a method, as described [here](https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentqueue-1.isempty?view=net-6.0). The suggested edit queue is full, so just leaving a comment. – skillz21 Jan 05 '22 at 06:33