Is this the correct object to lock on for thread safety?

Question

The code below is going to be called from a website so having a static dictionary object in a non static class needs to be threadsafe. Basically the purpose of the code is to encapsulate logic and to maintain the life-time of the perfmon counter(s), which are stored in an instance of CounterContainer. The constructor is called passing in instanceName. The constructor needs to check to see if a CounterContainer for that instanceName has been defined and stored in the dictionary. If so, it can (and must) use that instance. If not it creates an instance of the CounterContainer, stores it in the dictionary and then uses that instance. The instance of the CounterContainer to be used is stored in a non static member so is thread safe from that point on.

As the only place that that the static dictionary is used is within the constructor it feels to me that it will be safe to lock on the dictionary for the duration that it will be accessed? Is this going to cause any unforeseen issues later on with anything like blocking / deadlocks? I can't see anything, but haven't really had the need to consider this sort of thing too much in the past.

I have also considered lock(this): but I don't think that would work as it would only lock the instance of PerformanceCounters being created, and not the underlying static dictionary (so would not be threadsafe).

namespace ToolKit
{
    using System;
    using System.Diagnostics;
    using System.Collections.Generic;

    public class PerformanceCounters : IPerformanceCounters
    {
        private static Dictionary<string, CounterContainer> _containers = new Dictionary<string, CounterContainer>();
        private CounterContainer _instanceContainer;

        public PerformanceCounters(string instanceName)
        {
            if (instanceName == null) throw new ArgumentNullException("instanceName");
            if (string.IsNullOrWhiteSpace(instanceName)) throw new ArgumentException("instanceName");

            // Is this the best item to lock on?
            lock (_containers)
            {

                if (_containers.ContainsKey(instanceName))
                {
                    _instanceContainer = _containers[instanceName];
                    return;
                }

                _instanceContainer = new CounterContainer(instanceName);
                _containers.Add(instanceName, _instanceContainer);
            }
        }
        public void Start()
        {
            _instanceContainer.AvgSearchDuration.Start();
        }

        public void FinishAndLog()
        {
            _instanceContainer.SearchesExecuted.Increment();
            _instanceContainer.SearchesPerSecond.Increment();
            _instanceContainer.AvgSearchDuration.Increment();
        }
    }
}

Answer 1

Not to disagree with the suggestion that ConcurrentDictionary be used, but to answer more generally:

1. The best locking solution is one that doesn't lock at all. Sometimes it's not very difficult to make something concurrent without locking. Next best is having fine-grained locks. However, coarse-grained locks are much easier to reason about, and hence to be confident in their correctness. Unless you've got a well-tested lock-free class of appropriate purpose to hand, start with coarse-grain locks (locks that block a whole bunch of operations with the same lock) and then move to finer-grained either because there is a logical deadlock (you can see that two unrelated operations are likely to block each other) or as an optimisation if needed.
2. Never lock on this because something external to your that code could lock on the object and you can easily get a deadlock or at least lock contention that can only be found by looking at both the code internal to the class, and the calling code (and the person writing one may not have access to the other). For similar reasons, never lock on a Type.
3. For similar reasons, only ever lock on a private member. That way only code internal to the class can lock on it, and the possibility of lock contention is restricted to that place.
4. Avoid use of partial with such code, and if you do use partial, try to keep all locks on an object in the same place. No technical reason here, but it does help when you need to think about all the potential places a lock could be taken.
5. Never lock on a value type (integer type, struct, etc.). Doing so will box the value to a new object and lock on this. Then when some other code tries to get the lock it will box to yet another object and lock on that. Essentially there is no lock at all (barring the theoretical optimisation, of boxing using a flyweight pattern [which it doesn't] but would actually make things even worse).
6. Sometimes the purpose of the lock will relate to a single object that is used when the lock is used, and not used when a lock isn't used. In this case, using this object as that to lock on helps the readability of the code, in associating the lock with that object.
7. It's never wrong to have an object that exists purely for the purpose of the lock, so if you are unsure, just do that.
8. The lock object must be static if any of the objects affected are static, and may be instance otherwise. Conversely, since an instance lock is more finely-grained than a static lock, instance is better when it's applicable.

Answer 2

Yes, _containers is theoretically ok as it is scoped the same as the dictionary instance, that is to say, it's a static (obviously - it IS the dictionary instance.)

However once thing worries about your overall design is that you say this is being hosted in a website. Each asp.net worker is a separate process so your static dictionary may be destroyed when IIS recycles the worker process due to idling or otherwise. Also, you may have more than one instance of your dictionary if you are using web gardens (multiple workers per application.)

Answer 3

Yes, it will work as _containers is static.

You might want to take a look at ReaderWriterLockSlim as well so you want block the theard so much (improvement to performance)

Answer 4

It seems to me quite common to have an explicit object instance that is used exclusively for locking.

private readonly object containerLock = new object();
...
lock (containerLock) {
 ...
}

Another tip: If you are working against .NET 4, consider using ConcurrentDictionary.