Why is it a bad practice to lock the object we are going to change?

Question

Why is it a bad practice to use lock as in the following code, I'm assuming this is a bad practice based on the answers in this SO question here

private void DoSomethingUseLess()
{
    List<IProduct> otherProductList = new List<IProduct>();
    Parallel.ForEach(myOriginalProductList, product =>
        {
           //Some code here removed for brevity
           //Some more code here :)
            lock (otherProductList)
            {
                otherProductList.Add((IProduct)product.Clone());
            }
        });
}

The answers over there mentions that it is bad practice , but they don't say why

Note: Please ignore the usefulness of the code, this is just for example purpose and i know it is not at all useful

Answer 1

From the C# language reference here:

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 because 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.

In your case, I would read the above guidance as suggesting that locking on the collection you will be modifying is bad practise. For example, if you wrote this code:

lock (otherProductList) 
{
    otherProductList = new List<IProduct>(); 
}

...then your lock will be worthless. For these reasons it's recommended to use a dedicated object variable for locking.

Note that this doesn't mean your application will break if you use the code you posted. "Best practices" are usually defined to provide easily-repeated patterns that are more technically resilient. That is, if you follow best practice and have a dedicated "lock object," you are highly unlikely to ever write broken lock-based code; if you don't follow best practice then, maybe one time in a hundred, you'll get bitten by an easily-avoided problem.

Additionally (and more generally), code written using best practices is typically more easily modified, because you can be less wary of unexpected side-effects.

Answer 2

It might be not a good idea indeed, because if someone else uses the same object reference to do a lock, you could have a deadlock. If there is a chance your locked object is accessible outside your own code, then someone else could break your code.

Imagine the following example based on your code:

namespace ClassLibrary1
{
    public class Foo : IProduct
    {
    }

    public interface IProduct
    {
    }

    public class MyClass
    {
        public List<IProduct> myOriginalProductList = new List<IProduct> { new Foo(), new Foo() };

        public void Test(Action<IEnumerable<IProduct>> handler)
        {
            List<IProduct> otherProductList = new List<IProduct> { new Foo(), new Foo() };
            Parallel.ForEach(myOriginalProductList, product =>
            {
                lock (otherProductList)
                {
                    if (handler != null)
                    {
                        handler(otherProductList);
                    }

                    otherProductList.Add(product);
                }
            });
        }
    }
}

Now you compile your library, send it to a customer, and this customer writes in his code:

public class Program
{
    private static void Main(string[] args)
    {
        new MyClass().Test(z => SomeMethod(z));
    }

    private static void SomeMethod(IEnumerable<IProduct> myReference)
    {
        Parallel.ForEach(myReference, item =>
        {
            lock (myReference)
            {
                // Some stuff here
            }
        });
    }
}

Then there could be a nice hard-to-debug deadlock for your customer, each of two used thread waiting for the otherProductList instance to be not locked anymore.

I agree, this scenario is unlikely to happen, but it illustrates that if your locked reference is visible in a piece of code you do not own, by any possible way, then there's a possibility for the final code to be broken.