CODEWITHSUNDEEP
c#multithreadingsynchronizationlocking
Cheetah
Cheetah

Reputation: 14449

Using a separate object for synchronisation

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");
}

Upvotes: 2

Views: 124

Answers (5)

Tilak
Tilak

Reputation: 30738

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:

  1. lock (this) is a problem if the instance can be accessed publicly.
  2. lock (typeof (MyType)) is a problem if MyType is publicly accessible.
  3. 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#

Upvotes: 2

Toan Nguyen
Toan Nguyen

Reputation: 11601

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();
}

Upvotes: 0

Yiğit Yener
Yiğit Yener

Reputation: 5986

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.

Upvotes: 0

humblelistener
humblelistener

Reputation: 1456

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.

Upvotes: 0

Rotem
Rotem

Reputation: 21947

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?

Upvotes: 5

Related Questions