.NET GC Accessing a synchronised object from a finalizer

Question

I recently read this article Safe Thread Synchronization as I was curious about the thread safety of calls made from a finaliser. I wrote the following code to test access to a static thread safe collection from a finaliser.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace GCThreadTest
{
    class Program
    {
        static class FinaliserCollection
        {
            private static Queue<int> s_ItemQueue = new Queue<int>();
            private static System.Object s_Lock = new System.Object();

            public static void AddItem(int itemValue)
            {
                lock(s_Lock)
                {
                    s_ItemQueue.Enqueue(itemValue);
                }
            }

            public static bool TryGetItem(out int item)
            {
                lock(s_Lock)
                {
                    if (s_ItemQueue.Count <= 0)
                    {
                        item = -1;
                        return false;
                    }

                    item = s_ItemQueue.Dequeue();
                    return true;
                }
            }
        }

        class FinaliserObject
        {
            private int m_ItemValue;

            public FinaliserObject(int itemValue)
            {
                m_ItemValue = itemValue;
            }

            ~FinaliserObject()
            {
                FinaliserCollection.AddItem(m_ItemValue);
            }
        }

        static void Main(string[] args)
        {
            int itemValueIn = 0;
            int itemValueOut = 0;

            while (itemValueOut < 10000)
            {
                System.Threading.ThreadPool.QueueUserWorkItem
                    (delegate(object value)
                    {
                        new FinaliserObject((int)value);

                        System.Threading.Thread.Sleep(5);

                    }, itemValueIn);

                itemValueIn = itemValueIn + 1;

                // This seems to stop finaliser from
                // being called?
                // System.Threading.Thread.Sleep(5);

                int tempItemValueOut = -1;
                if (FinaliserCollection.TryGetItem(out tempItemValueOut))
                    itemValueOut = tempItemValueOut;
            }

            System.Console.WriteLine("Finished after {0} items created", itemValueOut);
            System.Console.ReadLine();
        }
    }
}

Without the 'Sleep' call in the while loop this code seems to run fine but is it really safe from deadlocking? Would it ever be possible for a finaliser call to be made while a queued thread pool item is accessing the static collection? Why does adding the 'Sleep' to the main threads while loop appear to stop all finalisers from being called?

Answer 1

Wow. What the... This is the most bizarre piece of code I've ever seen. @.@

First of all, what finalizer call are you referring to? The only finalizer I see is the finalizer for the FinaliserObject, which will be called 10,000 times, and can be called independently of whatever's going on on the static collection. I.E. yes, those objects can be destroyed while other objects are being dequeued from the collection. This isn't an issue.

The static collection itself won't be cleaned up until the app itself exits.

Keep in mind that there's absolutely no guarantee when or if those finalizers will be called before the app itself exits. Your static collection could be completely empty when you exit.

Worse, you're assigning itemValueOut to whatever the last value you pull out of the queue is... which is NOT the number of items created, as you imply in your WriteLine(). Because those destructors are called in any possible order, you could theoretically add to the queue 10,000, 9,999, 9,998, ... 2, 1, in that order.

Which is further an issue, because you're removing from the queue 10,000 times, but on the last loop, it's very possible there won't be an object to dequeue, in which case you're guaranteed to get -1 for the number of items returned (even if the other 9,999 items worked successfully).

To answer your question, this code cannot deadlock. A deadlock would happen if AddItem() called TryGetItem(), but those locks are pretty much guaranteed to keep each other out of the static collection while adding or removing items.

Where you're tempting fate is that you can exit your app without all of the FinaliserObjects having added themselves to the queue. Meaning one of the finalizers could fire and try to add to the FinaliserCollection, but the FinaliserCollection has already been disposed. What you're doing in the finaliser is terrible.

But yes, a finalizer call can happen while you're calling FinaliserCollection.TryGetItem(). The finalizer will block and wait until TryGetItem() emerges from the lock(), at which point it will add another item. This is not an issue.

As for the sleep() command, you're probably just throwing the timing of the garbage collection off. Remember, your objects won't be collected/finalized until the GC decides it needs the resources.

Sorry for being so emphatic... I know you're just trying to test a concept but I really don't understand why you would want to do what you're trying to do in the finalizer. If there's really a legitimate goal here, doing it in the finalizer is not the correct answer.

Edit

From what I'm reading and what Sasha is saying, no you will not have a deadlock. The finalizer thread may be blocked waiting for the lock, but the GC will not wait for the finalizer, and will thus unsuspend the threads, allowing the locks to be released.

In any case, this is a very strong argument for why you shouldn't be making calls like this in a finalizer... the finalizer is only for releasing unmanaged resources. Anything else is playing roulette.