Using a separate object for synchronisation

Question

I see this a lot:

object lockObj;
List<string> myStrs;

// ...

lock(lockObj)
{
    myStrs.Add("hello world");
}

Why have the separate object? Surely you can just do this:

List<string> myStrs;

// ...

lock(myStrs)
{
    myStrs.Add("hello world");
}

score 5 · Answer 1 · edited May 23 '17 at 11:43

5

It is a problem to lock directly on the list only if myStrs is public, and thus can be locked by other callers as well, resulting in a possible deadlock.

If it is a private member, then there should be no problem, but locking on a seperate object is a good habit in any case.

See this similar question for a more detailed answer: Why is lock(this) {...} bad?

edited May 23 '17 at 11:43

Community

1
1

answered Dec 28 '12 at 13:49

Rotem

21,452
6
62
109

score 2 · Answer 2 · answered Dec 28 '12 at 14:05

In general, avoid locking on a public type, or instances beyond your code's control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:

lock (this) is a problem if the instance can be accessed publicly.

lock (typeof (MyType)) is a problem if MyType is publicly accessible.

lock(“myLock”) is a problem since any other code in the process using the same string, will share the same lock.

Best practice is to define a private object to lock on, or a private static object variable to protect data common to all instances.

Form the documentation lock c#

+1 - These are the big issues - mainly the whole "if somebody else can see the thing I'm locking on, somebody else can snatch my lock" point; Doubly dangerous to use a string as mentioned here, since strings are generally interned such that all equal strings are effectively the same object. — JerKimball, Jan 02 '13 at 19:55

Yiğit Yener · Answer 3 · 2012-12-28T14:03:11.967

0

Your list of string is a list used for internal implementation details.

The problem with the second version could rise if you change your implementation such a way that you re-initilazie the strings list.

Then the thread-safety of your implementation could be broken.

So it's better that you use a seperate object for synchronization and declare this object as read only.

edited Dec 28 '12 at 14:03

answered Dec 28 '12 at 13:51

Yiğit Yener

5,796
1
23
26

score 0 · Answer 4 · answered Dec 28 '12 at 13:55

The idea is to always lock a private members that can only be accessed by the code that we are looking at. Whereas when we lock the members that we do not have control upon like public member or similar, chances are some other part of code can already hold a lock. This could lead to unexpected blocking behavior.

So, I think this had lead to thumb rule / best practice of having a private object especially for locking.

I would be interested in seeing if there are more reasons coming up.

score 0 · Answer 5 · answered Dec 28 '12 at 14:04

If you use the list as the lock object, and it is reset to null, then the lock(myStringList) will throw an ArgumentNullException. Below the simple test code of a console application.

  private static IList<string> mystringList = new List<string>();

static void Main(string[] args)
{
    new Thread(() =>
        {
            try
            {

                while (true)
                {
                    //Acquire the lock
                    lock (mystringList)
                    {
                        //Do something with the data
                        Thread.Sleep(100);
                        Console.WriteLine("Lock acquired");
                    }
                }
            }
            catch (Exception exception)
            {
                Console.WriteLine("Exception: " +exception.Message);
            }
        }).Start();

    new Thread(() =>
        {
            //Suppose we do something
            Thread.Sleep(1000);

            //And by some how reset the list to null
            mystringList = null;

        }).Start();

    Console.ReadLine();
}

Using a separate object for synchronisation

5 Answers5