33

I have a situation where very rarely a Queue of Objects is dequeuing a null. The only call to Enqueue is within the class itself:

m_DeltaQueue.Enqueue(this);

Very rarely, a null is dequeued from this queue in the following code (a static method):

while (m_DeltaQueue.Count > 0 && index++ < count)
    if ((m = m_DeltaQueue.Dequeue()) != null)
        m.ProcessDelta();
    else if (nullcount++ < 10)
    {
        Core.InvokeBroadcastEvent(AccessLevel.GameMaster, "A Rougue null exception was caught, m_DeltaQueue.Dequeue of a null occurred. Please inform an developer.");
        Console.WriteLine("m_DeltaQueue.Dequeue of a null occurred: m_DeltaQueue is not null. m_DeltaQueue.count:{0}", m_DeltaQueue.Count);
    }

This is the error report that was generated:

[Jan 23 01:53:13]: m_DeltaQueue.Dequeue of a null occurred: m_DeltaQueue is not null. m_DeltaQueue.count:345

I'm very confused as to how a null value could be present in this queue.

As I'm writing this, I'm wondering if this could be a failure of thread synchronization; this is a multi threaded application and It's possible the enqueue or dequeue could be happening simultaneously in another thread.

This is currently under .Net 4.0, but it previously occurred in 3.5/2.0

Update:

This is my (hopefully correct) solution to the problem which was made clear though the great answers below as being a synchronization problem.

private static object _lock = new object();
private static Queue<Mobile> m_DeltaQueue = new Queue<Mobile>();

Enqueue:

    lock (_lock)
        m_DeltaQueue.Enqueue(this);

Dequeue:

       int count = m_DeltaQueue.Count;
       int index = 0;
       if (m_DeltaQueue.Count > 0 && index < count)
           lock (_lock)
               while (m_DeltaQueue.Count > 0 && index++ < count)
                   m_DeltaQueue.Dequeue().ProcessDelta();

I'm still trying to get a handle on proper syncronization, so any comments on the correctness of this would be very appreciated. I initially chose to use the queue itself as a syncronization object because it's private, and introduces less clutter into what is already a very large class. Based on John's suggestion I changed this to lock on a new private static object, _lock.

8
  • 5
    "I'm wondering if this could be a failure of thread synchronization;" - Yep. 99.99 % guaranteed to be a synchronisation issue (race condition) Feb 20, 2011 at 3:35
  • Thanks so much for the great answers. I've been dealing with this issue for months. I guess that's the problem with sync errors in that they don't often present themselves, aren't reproducable, and don't make any sence. In all the time I've spent thinking about this it didn't strike me that it could be threading until i started asking the question.
    – Derrick
    Feb 20, 2011 at 3:47
  • 2
    @Derrick: what you should just have learned is to not use threads unless you have to, and unless you are at the point where you can just glance at code and see the problems. As soon as you said it was a multi-threaded application, it was obvious that you had a race condition! You should stay away from threads until you understand why this makes perfect sense. Feb 20, 2011 at 3:49
  • You should not lock on m_DeltaQueue. Lock on a separate lock object which is "object for locking all the stuff in this class". Feb 20, 2011 at 17:22
  • 1
    @CodeInChaos: my concern is that he needs to lock entire operations, not just the queue. I'm concerned about access to m_DeltaQueue.Count, for instance. I'd be more comfortable with a lock around a larger amount of code, at least until performance analysis showed me a problem. Less to have to think about. Feb 20, 2011 at 20:11

6 Answers 6

35

this can never be null, unless the method was called using a call instruction in hand-written IL.

However, if you use the same Queue instance on multiple threads simultaneously, the queue will become corrupted and lose data.

For example, if two items are added simultaneously to a near-capacity queue, the first item might be added to the array after the second thread resizes it, which will end up copying a null to the resized array and adding the first item to the old array.

You should protect your queues with locks or use .Net 4's ConcurrentQueue<T>.

6
  • 2
    Is there a performance hit in using ConcurrentQueue over locks? ConcurrentQueue seems like the easy way out. Thanks for the great example of how this can happen.
    – Derrick
    Feb 20, 2011 at 3:50
  • IIRC even the hand-written IL usually generates a "could destabilise the runtime" error Feb 20, 2011 at 8:19
  • 6
    My guess it that ConcurrentQueue might even be a bit faster than manual locking since it probably uses lock free programming techniques. Your use should probably use the TryDequeue method. Feb 20, 2011 at 16:53
  • 2
    @CodeInChaos - in many situations a lock-free algorithm will out perform a solution involving explicit locking but it's not a given (can anyone find a good ref? I googled but didn't find one). However using a well tested lock-free solution is almost certainly less error prone than coding your own locking around a non-thread safe data structure. And until it's shown that there is a performance issue, one way or another in a given situation, this would always drive my choice. Feb 21, 2011 at 8:23
  • IL isn't the only .NET language that can call a method on null - F# can. @GeorgeHawkins if you want a case where lock-based beats lock-free, the time a lock around a Queue for a single thread vs. a ConcurrentQueue. Then consider that lots of times when we have concurrency we don't have heavy concurrency - we've only one lock at a time 99% of the calls but have to protect against the 1%. The fact that locked Queue beats ConcurrentQueue for 1 thread shows it'll out-perform in that low-concurrency case.
    – Jon Hanna
    Aug 18, 2012 at 17:28
3

this can never be null (the CLR will raise an exception if you try to call a method on null). It's almost certainly the case that you have a synchronization bug, where two threads are trying to add to the queue simultaneously. Perhaps both threads are incrementing an index into the array and then putting their value into the same location. This means that the first thread is getting its value overwritten.

Either synchronize your access (e.g. with lock) or use a ConcurrentQueue (in .Net 4).

1
  • 1
    That's not quite true. A call (not callvirt) instruction doesn't have a null check. Jon Skeet investigated this.
    – SLaks
    Feb 20, 2011 at 19:59
3

Queues are not inherently thread safe. This is your issue. Use a mutex/lock/whatever or look for a thread safe-queue.

3

Indeed, if the Queue class you are using is not thread safe, you could be Dequeueing from two threads at the same time. The easiest way to avoid this is by locking your queue when you are dequeueing from it.


//declare this object in a globally accessible location
object locker = new object();

lock(locker)
{
    m = mDeltaQueue.Dequeue();
}
7
  • @John my bad. I wasn't thinking. I've edited appropriately. The locker object should be visible from all threads that are using the queue.
    – Darkhydro
    Feb 20, 2011 at 3:49
  • I copied the code! I was suprised that lock was being used without an object. I'm reading up now on the proper way to lock the queue. (I'm assuming Enqueue needs to be locked on the same object, and that that object should not be m_DeltaQueue>)
    – Derrick
    Feb 20, 2011 at 3:58
  • 1
    @Derrick: how long did you have this problem? Any way you can just avoid threads? Feb 20, 2011 at 4:48
  • It's happened 4 or 5 times over the past 8 months; only once in the 2.5 months since I added this logging code; previously it was throwing an null-exception. This enqueue should not actually be occurring in another thread so I'm also trying to track down the why/how on what is triggering it. It's likely a property change from another comparatively inactive thread that triggers an Enqueue (likely there is some poor,short-sighted code in there somewhere). The Dequeue thread is very very busy, so as I've been looking into the ConcurrentQueue option, the SpinWait within is a little scary to me.
    – Derrick
    Feb 20, 2011 at 5:09
  • 1
    @Derrick I apologize for providing an a corrupted code sample. I should have been more thorough.
    – Darkhydro
    Feb 20, 2011 at 5:47
1

(Slightly off-topic and a highly unlikely possibility; have made this community wiki. The real question has already been resolved; this is mainly related to the title of the question.)

In theory, if your code m_DeltaQueue.Enqueue(this) resulted in the invocation of an implicit conversion operator on the argument, that could indeed result in a null-reference being passed to the method.

class Foo
{
    public static implicit operator string(Foo foo)
    {
        return null;
    }

    void InstanceMethod()
    {
        string @this = this;

        if (@this == null)
            Console.WriteLine("Appears like 'this' is null.");
    }

    static void Main()
    {
        new Foo().InstanceMethod();
    }
}
2
  • 1
    That is an interesting trick; I think it's a good note to have here in the context of the original question. Thanks.
    – Derrick
    Feb 21, 2011 at 2:25
  • Technically this isn't null in this case, you've just created a new variable based on this that happens to be null.
    – Servy
    Jan 30, 2013 at 21:14
1

It is possible to create a delegate that calls an instance method on a null instance using the Delegate.CreateDelegate(Type, object, MethodInfo) overload.

The MSDN says (emphasis mine)

If firstArgument is a null reference and method is an instance method, the result depends on the signatures of the delegate type type and of method:

  • If the signature of type explicitly includes the hidden first parameter of method, the delegate is said to represent an open instance method. When the delegate is invoked, the first argument in the argument list is passed to the hidden instance parameter of method.
  • If the signatures of method and type match (that is, all parameter types are compatible), then the delegate is said to be closed over a null reference. Invoking the delegate is like calling an instance method on a null instance, which is not a particularly useful thing to do.

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.