C# Static Property Locking

Question

Just looking for a code review of this code. ASP.net Cache is not an option. The static list will be accessed a lot on a website that gets well over 10K page views per day and concurrent read attempts is likely. On app restart when the list is rebuilt I was wondering if there are any issues I may be overlooking? Is locking on the list being instantiated best practice?

public class MyClass
{
        private static List<Entry> _listCache = null; 
        protected static List<Entry> ListCache
        {
            get
            {

                if (_listCache == null)
                {
                    _listCache = new List<Entry>();
                    lock (_listCache)
                    {
                        //Add items to the list _listCache from XML file
                    }
                }
                return _listCache;
            }
        }
        //....Other methods that work with the list
}

Answer 1

Multiple threads can initalize _listCache. Depending on the code generation optimizations and runtime execution optimization, this may result in multiple threads locking and updating different objects. And besides, you can't expose the list as a property allowing anyone to adds/remove object w/o a lock.

You'd be better using an immutable list that multiple readers can safely parse in read-only mode. Alternatively you can use a Read-Write lock, but things will get pretty hairy, between the initialization control and the access r-w control.

Answer 2

10k views - that's one every 8 seconds... not sure you need to worry too much... ;-p

But re the code - that is overcomplicating things, and you could still end up initializing it twice. I'd just use a static constructor to do this; it'll be more robust. If you must have full isolated lazy loading (even with other static methods on the type), there is a trick with an inner class to achieve the same:

public class MyClass
{
    static class InnerCache {
        internal static readonly IList<Entry> _listCache;
        static InnerCache() {
            List<Entry> tmp  = new List<Entry>();
            //Add items to the list _listCache from XML file
            _listCache = new ReadOnlyCollection<Entry>(tmp);
        }
    }
    protected static IList<Entry> ListCache {
        get {return InnerCache._listCache;}
    }
}

I would also be concerned about the chance of somebody mutating the list - might want to use a readonly list!

Answer 3

A static constructor may be your best bet here. A static constructor will block all threads that depend on it while it's running, and it will only run once. As you have the code here, the lock doesn't really do anything, and there are lots of ways that bad things can happen, including multiple Lists being initialized from XML at the same time. In fact, one thread could create a new List then lock and load a different list and then return a third list, depending on when the thread switching occurs.

Answer 4

There's not really a reason this wouldn't work for you. However, if you want to do it the way your sample code is, you want to lock before you check to see if _listCache is null. So you would need a separate monitor to lock on. Something like this:

public class MyClass
{
        private static object _listCacheMonitor = new object();
        private static List<Entry> _listCache = null; 
        protected static List<Entry> ListCache
        {
            get
            {
                lock (_listCacheMonitor) {    
                    if (_listCache == null)
                    {
                        _listCache = new List<Entry>();
                        //Add items to the list _listCache from XML file
                    }
                }
                return _listCache;
            }
        }
        //....Other methods that work with the list
}