Can I lock a collection in the get accessor?

Question

I have a lightly used dictionary which is hardly ever going to be read or updated since the individual items raise events and return their results with their event args. In fact the thread is always going to be updated with the same thread. I was thinking about adding a simple lock just to be safe. I was wondering if I can just place the lock in the get accessor. Does this work?

        Dictionary<string,Indicator> indicators = new Dictionary<string,Indicator>();
        Dictionary<string, Indicator> Indicators
        {
            get
            {
                lock (indicators)
                {
                    return indicators;
                }
            }
        }

        public void AddIndicator(Indicator i)
        {
            lock (indicators)
            {
                indicators.Add(i.Name, i);
            }
        }

Answer 1

That doesn't do anything particularly useful, no.

In particular, if you have:

x = foo.Indicators["blah"]

then the indexer will be executed without the thread holding the lock... so it's not thread-safe. Think of the above code like this:

Dictionary<string, Indicator> indicators = foo.Indicators;
// By now, your property getter has completed, and the lock has been released...
x = indicators["blah"];

Do you ever need to do anything with the collection other than access it via the indexer? If not, you might want to just replace the property with a method:

public Indicator GetIndicator(string name)
{
    lock (indicators)
    {
        return indicators[name];
    }
}

(You may want to use TryGetValue instead, etc - it depends on what you're trying to achieve.)

Personally I'd prefer to use a reference to a privately-owned-and-otherwise-unused lock object rather than locking on the collection reference, but that's a separate matter.

As mentioned elsewhere, ConcurrentDictionary is your friend if you're using .NET 4, but of course it's not available prior to that :(

Answer 2

Other than Jon's input, I'll say don't lock the collection indicators itself anyway, from MSDN:

Use caution when locking on instances, for example lock(this) in C# or SyncLock(Me) in Visual Basic. If other code in your application, external to the type, takes a lock on the object, deadlocks could occur.

It is recommended to use a dedicated object instance to lock onto. There are other places where this is covered with more details and reasons why - even here on SO, should you care to search for the information when you have time.