Can I lock a collection in the get accessor?

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. 开发者_JS百科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);
            }
        }

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 :(

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.

Alternatively, you could use ConcurrentDictionary which handles the thread safety for you.

Short answer: YES.
Why shouldn't that work, but as mention by Jon, it does not lock as intended when using indexes?