Is my use of ConcurrentQueue here between 2 threads ok?

Question

Hi,

Is my use of ConcurrentQueue here between 2 threads ok? I wanted to check I don't need to "lock" anywhere explicitly. In particular look at the lines where I have in COMMENTS would I drop a packet here...

public class PacketCapturer
{

private static ConcurrentQueue<Packet> _packetQueue = new ConcurrentQueue<Packet>();

public PacketCapturer(IPHostEntry proxyDns, ref BackgroundWorker bw)
{
    // start the background thread
    var backgroundThread = new System.Threading.Thread(BackgroundThread);
    backgroundThread.Start();

    // Start Packet Capture with callback via PacketCapturerCallback
}

private void PacketCapturerCallback(Packet packet)
{
    _packetQueue.Enqueue(packet);
}

private static void BackgroundThread()
{
    while (!BackgroundThreadStop)
    {
        if (_packetQueue.Count == 0) Thread.Sleep(250);
        else 
        {
            ConcurrentQueue<Packet> ourQueue;  
            ourQueue = _packetQueue;   // COULD I DROP A PACKET BETWEEN HERE
            _packetQueue = new ConcurrentQueue<Packet>();      // AND HERE???

            Console.WriteLine("BackgroundThread: ourQueue.Count is {0}", ourQueue.Count);
        }
    }
}

Answer 1

Edit: I think Mark/Gabe are right that you won't lose any Packets. I'll leave the rest in just for reference in case anyone else can weigh in on this.

---

Simply, yes. You could lose one or more Packets there. You might want to look into what methods ConcurrentQueue offers to get/remove a portion of it as that looks like what you want to do.

Why not TryDequeue until it returns false:

    else 
    {
        Queue<Packet> ourQueue = new Queue<Packet>();  //this doesn't need to be concurrent unless you want it to be for some other reason.
        Packet p;
        while(_packetQueue.TryDequeue(out p))
        {
            ourQueue.Enqueue(p);
        }

        Console.WriteLine("BackgroundThread: ourQueue.Count is {0}", ourQueue.Count);
    }

Answer 2

No, is not OK. First of all, if you change the reference like this in a concurrent thread, the _packetQueue must be marked volatile to prevent compiler and codegen optimizations from never seeing the change. The cahnging of the _packetQueue shoudl normally occur as an Interlocked.CompareExchange, but this is less critical for your usage.

But for more worying is the pattern of changing the packetQueue instance like this in the background thread. What is the purpose of this? It has a horible code smell...

updated

What I ususaly do is this:

Producer thread(s):

Producer () {
...
lock(_sharedQueue) {
  _sharedQueue.Enqueue(something);
}
...
}

consumer thread:

consumer (...) {
...
var Something[] toProcess = null;
lock(_sharedQueue)
{
  toProcess = _sharedQueue.Toarray();
   _sharedQueue.Clear();
}
// Process here the toProcess array
...
}

This was good enough for every single use I ever had. Processing does not occur under the lock so locking is minimal. There is no need for the fancy ConcurrentQueue, a plain old .Net 2.0 collection is enough. Usually I use a dedicated locking object, for good practice, instead of locking the actual queue instance.

Answer 3

I don't think you could drop a packet, as obviously any thread is going to dereference to either the old or new. The edge-case is that you get more data arrive in ourQueue after you think you've swapped them, or you could also end up not noticing the reference change in the Sleep loop (but: with 2 threads, the only thread that changes the reference is this thread, so not necessarily a problem).

TBH, if you have 2 threads here, why not just lock or use ReaderWriterLock? It will be a lot simpler to figure out what happens when...