Locking on the object that is being synchronized or using a dedicated lock object?

Question

As far as I've understood from colleagues and the web, it is bad practice to lock on the object that is being synchronized, but what I dont understand is why?

The following class is supposed to load settings to a dictionary, and it has a method to retrieve settings as well.

public class TingbogSettingService : ITingbogSettingService
{
    private readonly ISettingRepository _settingRepository;
    private readonly ICentralLog _centralLog;
    private Dictionary<string, ISetting> _settingDictionary = new Dictionary<string, ISetting>();


    public TingbogSettingService(ISettingRepository settingRepository, ICentralLog centralLog)
    {
        _settingRepository = settingRepository;
        _centralLog = centralLog;
    }

    public ISetting GetSetting(string settingName)
    {
        ISetting setting;
        if (!_settingDictionary.TryGetValue(settingName, out setting))
        {
            return null;
        }
        return setting;
    }

    public void LoadSettings()
    {
        var settings = _settingRepository.LoadSettings();
        try
        {
            lock (_settingDictionary)
            {
                _settingDictionary = settings.ToDictionary(x => x.SettingName);
            }                
        }
        catch (Exception ex)
        {
            _centralLog.Log(Targets.Database, LogType.Error, $"LoadSettings error: Could not load the settings", new List<Exception>() { ex });
        }
    }
}

During the LoadSettings function I want to lock the _settingDictionary, so that GetSetting will be blocked, until the new settings are loaded.

Should I use a dedicated lock object instead?

For instance:

private static readonly object m_Lock = new object();
…
lock (m_Lock)

EDIT
I thought that lock(_settingDictionary) would lock the _settingDictionary itself, however I now realize that his is not the case. What I wanted was to prevent other threads from accessing _settingDictionary until the new settings were loaded (LoadSettings method completed). As only 1 thread is updating the _settingDictionary, I guess I dont need a lock there at all.

As for closing the question - something similar has been asked before, yes, but the scenario is not the same. I learned from your answers and it is going to be hard to pick a winner amongst y'all.

Answer 1

This is quite a broad subject, but let me focus on one major problem in your code: _settingDictionary changes.

You don't lock on the field, you lock on the instance. This means that when you lock on _settingDictionary, and then you change _settingDictionary, you're not preventing any concurrent access - anyone can lock on the new _settingDictionary.

lock doesn't prevent access to the object you're locking either. If you need synchronization, you must synchronize all access to the object, including your _settingDictionary.TryGetValue. Dictionary isn't thread-safe.

The main guide-lines to what you should lock on are something like this:

• The lock object is private to the locker - if it's not private, some other class might be holding a lock on your object, which may lead to deadlocks.
• The field should be readonly - this is not a strict requirement, but it makes things easier. The main point is that you must not lock on an object that might change while the lock is being held; others trying to take the lock concurrently will succeed.
• The lock object is a reference type - this kind of goes without saying, but you cannot lock on e.g. an int field, since it is boxed when you try to lock it - in effect, this is the same as the previous point - everyone locks on their own instance of the object, eliminating all synchronization.

Obligatory disclaimer: Multi-threading is hard. Seriously hard. Make sure you understand what's happening and what can possibly happen. Any multi-threaded code you write must be written in a way that's correct, first and foremost. http://www.albahari.com/threading/ is a great starter on all things multi-threaded in C#/.NET.

Answer 2

The problem you are talking about happens when you lock on an object to which external users of your class have access - most commonly, the object itself, i.e. lock (this).

If your code were locking on this instead of _settingDictionary, someone else could deadlock your code as follows:

TingbogSettingService svc = ...
lock (svc) {
    Task.Run(() => {
        svc.LoadSettings();
    });
}

When you lock on a private object, such as _settingDictionary in your case, there harmful effect described above is avoided, because nobody outside your code can lock on the same object.

Note: Using the lock in your code does not make it thread-safe, because GetSetting method does not lock on _settingDictionary when reading from it. Moreover, the fact that you re-assing _settingDictionary inside the lock makes locking irrelevant, because after the reassignment another thread can enter protected section in the lock.

Answer 3

There is no "right" or "wrong" answer to this but there are some guidelines and some things to be aware of.

First, there's many that feel that Microsoft should never have allowed to lock on arbitrary objects. Instead they should've encapsulated the locking functionality into a specific class and avoided potential overhead in every other object out there.

The biggest problem with allowing locking on arbitrary objects is that if you lock on an object you make publicly available to 3rd party code, you have no control over who else might be locking on the same object. You could write your code to the letter, dotting every I and it would still end up deadlocking because some other, 3rd party, code is locking on the same object out of your control.

So that point alone is guideline enough to say "don't ever lock on objects you make publicly available".

But what if the object you want to synchronize access to is private? Well, then it becomes more fuzzy. Presumably you have full control over the code you write yourself and thus if you then lock on the dictionary, as an example, then it will work just fine.

---

Still, my advice would be to always set up a separate object to lock on, get into this habit, and then you won't so easily make mistakes if you later decides to expose a previously private object into the public and forgetting to separate the locking semantics from it.

The simplest locking object is just that, an object:

private readonly object _SomethingSomethingLock = new object();

---

Also know, though I think you already do, that locking on an object does not "lock the object". Any other piece of code that doesn't bother with locks can still access the object just fine.

---

Here is also something I just noticed about your code.

When you do this:

lock (x)

You don't lock on x, you lock on the object that x refers to at the time of the lock.

This is important when looking at this code:

lock (_settingDictionary)
{
    _settingDictionary = settings.ToDictionary(x => x.SettingName);
}                

Here you have two objects in play:

1. The dictionary that settingDictionary refers to at the time of lock (_settingDictionary)
2. The new dictionary that .ToDictionary(...) returns

You have a lock on the first object, but not on the second. This is another scenario where having a dedicated locking object would not only make sense, but also be correct, as the above code is buggy in my opinion.

Answer 4

It is better to use a dedicated object that is not modified by the block of code or used for other purposes in some other methods. That way the object has a single responsibility so that you don't mix the usage of it as a synchronization object, with it being maybe set to null at some point or reinitialized by another method.

lock (_settingDictionary) doesn't lock the dictionary specified between (), it locks the next block of code by using _settingDictionary as a synchronization object (To know if the block has been entered of left by another thread by setting some flags on that object).

Answer 5

There are different thing you could lock:

• a dedicated non static object: private readonly object m_Lock = new object();
• a dedicated static object (your example): private static readonly object m_Lock = new object();
• the object itself: lock (_settingDictionary)
• this, typeof(MyClass)...

The first two are OK but actually different. Locking on a static object means the lock is shared between all instances of your classes. Locking on a non-static object means the lock is different for each instance of your class.

The third option is OK, it's the same as the first one. The only difference is that the object is not read-only (using a read-only field is slightly better as you ensure it won't ever change).

The last option is a bad option for various reasons, see Why is lock(this) {...} bad?

So be careful about what you lock, your example uses a static object while your initial code uses a non-static object. Those are really different use cases.