18

I'm have a wierd error in my code. It's extremely rare (happens once every few weeks maybe), but it's there and I'm not sure why.

We have 2 threads running, 1 thread gets networked messages and adds them to a Queue like this:

DataMessages.Enqueue(new DataMessage(client, msg));

Another thread takes messages off this queue and handles them, like this:

while (NetworkingClient.DataMessages.Count > 0)
{
    DataMessage message = NetworkingClient.DataMessages.Dequeue();

    switch (message.messageType)
    {
       ...
    }
}

However once every so often I get a NullReferenceException on the line switch (message.messageType) and I can see in the debugger that message is null.

It is not possible that a null value was put onto the queue (see the first bit of code), and these are the only 2 things that use the queue.

Is Queue not thread-safe, could it be that I'm dequeuing at the exact moment that the other thread is enqueuing and this causes the glitch?

3
  • very few things in the .NET BCL are thread-safe in this fashion. In this case, "to guarantee the thread safety of the Queue, all operations must be done through the wrapper returned by the Synchronized method". Your problem could also be that two threads are enqueueing or dequeueing simultaneously, if applicable. Either way, thread-safety is your responsibility.
    – bzlm
    Commented Jun 18, 2011 at 17:10
  • +1, Good question, it really demonstrates a race condition and its outcome (corrupted/unstable/unexpected situation).
    – Ron Klein
    Commented Jun 18, 2011 at 17:16
  • By the looks of your code, it looks like you're just doing a naive loop for your threads. You should look into implementing a proper bounded-buffer to synchronize this. Commented Jun 19, 2011 at 19:42

3 Answers 3

11

Is Queue not thread-safe, could it be that I'm dequeuing at the exact moment that the other thread is enqueuing and this causes the glitch?

Exactly. Queue is not thread-safe. A thread-safe queue is System.Collections.Concurrent.ConcurrentQueue. Use it instead to fix your problem.

1
  • 2
    Don’t use ConcurrentQueue blindly; only use it if you know what you’re doing. In some situations a Queue with locks is a better choice. The concurrent collection classes are not miracle cures for concurrency woes :)
    – Timwi
    Commented Jun 18, 2011 at 20:04
9
    while (NetworkingClient.DataMessages.Count > 0)
    {
        // once every two weeks a context switch happens to be here.
        DataMessage message = NetworkingClient.DataMessages.Dequeue();

        switch (message.messageType)
        {
           ...
        }
    }

... and when you get that context switch in that location, the result of the first expression (NetworkingClient.DataMessages.Count > 0) is true for both threads, and the one that get's to the Dequeue() operation first get's the object and the second thread get's a null (instead of InvalidOperationException because the Queue's internal state wasn't fully updated to throw the right exception).

Now you have two options:

  1. Use the .NET 4.0 ConcurrentQueue

  2. Refactor your code:

and make it look somehow like this:

while(true)
{
  DataMessage message = null;

  lock(NetworkingClient.DataMessages.SyncRoot) {
       if(NetworkingClient.DataMessages.Count > 0) {
          message = NetworkingClient.DataMessages.Dequeue();
       } else {
         break;
       }
    }
    // .. rest of your code
}

Edit: updated to reflect Heandel's comment.

1
  • the question does say "Another thread takes messages off this queue and handles them". This would mean a simultaneous dequeue from 2 threads would never happen. #justsaying
    – bzlm
    Commented Jun 18, 2011 at 17:43
8

In case you are interested in the exact reason:

Enqueue looks like this:

this._array[this._tail] = item;
this._tail = (this._tail + 1) % this._array.Length;
this._size++;
this._version++;

And Dequeue like this:

T result = this._array[this._head];
this._array[this._head] = default(T);
this._head = (this._head + 1) % this._array.Length;
this._size--;
this._version++;

The race goes like this:

  • There is 1 element in a queue (head == tail) so your reader thread starts dequeuing but is interrupted after the first line in Dequeue
  • Then another element is enqueued and put at position tail which is equal to head at this point.
  • Now Dequeue resumes and overwrites the element which was just inserted by Enqueue with default(T)
  • The next time you call dequeue you get the default(T) (in your case null) instead of the actual value
1
  • 1
    +1, it's nice to see exactly what is happening behind the scenes. I guess I should be more careful when multithreading! I'm glad I found this bug and it won't make it into the release.
    – Hannesh
    Commented Jun 19, 2011 at 22:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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