What is wrong with locking non-static fields? What is the correct way to lock a particular instance?

Question

Why is it considered bad practice to lock non-static fields?

And, if I am not locking non-static fields, then how do I lock an instance method without locking the method on all other instances of the same or derived class?

I wrote an example to make my question more clear.

public abstract class BaseClass
{

    private readonly object NonStaticLockObject = new object();
    private static readonly object StaticLockObject = new object();

    protected void DoThreadSafeAction<T>(Action<T> action)
        where T: BaseClass
    {
        var derived = this as T;
        if(derived == null)
        {
            throw new Exception();
        }
        lock(NonStaticLockObject)
        {
            action(derived);
        }
    }
}
public class DerivedClass :BaseClass
{
    private readonly Queue<object> _queue;
    public void Enqueue(object obj)
    {
        DoThreadSafeAction<DerivedClass>(x=>x._queue.Enqueue(obj));
    }
}

If I make the lock on the StaticLockObject, then the DoThreadSafeAction method will be locked for all instances of all classes that derive from BaseClass and that is not what I want. I want to make sure that no other threads can call a method on a particular instance of an object while it is locked.

Update

Thank you all for your help : I have posted a another question as a follow up to some of the information you all provided. Since you seem to be well versed in this area, I have posted the link: What is wrong with this solution to locking and managing locked exceptions?

Answer 1

It's not about it being bad-practice, it's about what is your purpose.

Static fields are accessed (or, "common to") all the instances of that type. So locking such an static field enables you to control concurrency between all the instances of that type, or, the scope of concurrency control achieved is all the instances of that type.

However, if you lock a non-static field, the lock will only be active for that instance, so you control concurrency only within that instance, or, the scope of concurrency control achieved is the instance.

---

Now, whenever locking an object I go like this. What is the resource that I'm concurring for? Maybe it's database, maybe it's a bunch of instance fields that can't be changed while I doing a certain processing, etc. Once I know what is I'm locking myself out of, I check it's scope.

1. If it's an entity outside my application, then it's application scope. Everything must be locked out simultaneously.
2. If it's a bunch of instance fields, then it's instance scope.
3. If it's a bunch of static fields, then it's type scope.

So, for 1 and 3, use a static field. For 2, use a instance field.

Now, another thing: usually, for 1, you will have a single class that wraps around that resource. And, usually you will design that class as a singleton. Now, with singletons, this is funny: you are guaranteed, by design, to have only a single instance, so it doesn't matter whether you are locking a instance or static field.

PS.: If you are using a lock to protect the instantiation of the singleton, of course it should be static. (why?)

Answer 2

You are locking an object that is used as a lock.

The difference therefor is where the lock is contained (or its accessibility).
If you have it as a static member, it is accessible to all the objects of the same class. So you get a single lock, that will lock them all.

If you have it as member of the class (non static) then it is only accessible to that object. So you will get a single lock per object instance.

There's no good-bad practice in this case. It's just a question of what you want to achieve.

Just remember to avoid locking this in an object.