2

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

5 Answers 5

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?

2

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#

1
  • +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 2, 2013 at 19:55
0

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.

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.

0

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.