Can I lock a collection in get accessor?

Question

I have an easily used dictionary that is unlikely to ever be read or updated, as individual elements raise events and return their results using their event arguments. In fact, a thread will always be updated with the same thread. I was thinking of adding a simple lock to be safe. I was wondering if I can just put the lock in a get accessor. It works?

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

It does nothing particularly useful, no.

In particular, if you have:

 x = foo.Indicators["blah"] 

then the index will execute without a thread holding the lock ... so it is not thread safe. Think about the code above, for example:

 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 a collection other than accessing it through an index? If not, you can simply replace the property with a method:

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

(Instead, you can use TryGetValue , etc. - it depends on what you are trying to achieve.)

Personally, I would prefer to use a link to a private and unused lock object, rather than link assembly, but this is a separate matter.

As mentioned elsewhere, ConcurrentDictionary is your friend if you are using .NET 4, but of course it is not available before that :(

Answer 2

With the exception of input to Jon, I’ll say that you don’t block the indicators collection itself from MSDN :

Use caution when locking instances, such as lock (this) in C # or SyncLock (Me) in Visual Basic. If other code in the application, outside the type, takes an object lock, deadlocks may occur.

To lock, it is recommended to use a dedicated instance of object . There are other places where this is described in more detail and why, even here at SO, you should look for information when you have time.

Answer 3

Alternatively, you can use ConcurrentDictionary , which will provide thread safety for you.

Answer 4

Short answer: YES.
Why should this not work, but, as John mentions, it does not block as intended when using indexes?