Locking method and calls of method for thread-safety in c#?

Question

Should I lock for threads a part of code that calling thread-safe method in c#?

For example:

T Eject (bool? state)
    {
        lock (this)
        {
            //somecode
            return _objects[i];
        }
    }

    public T TakeNew()
    {
        return Eject(null);
    }

    public T Reserve()
    {
        return Eject(true);
    }

Or I Should to lock return like this:

lock(this)
    {       
        return Eject(true);
    }

If i lock all calls the Eject(true), does I need to lock block of method Eject? I think, if I lock method and calls of method, its create a deadlock of threads. What is better way to create a thread-safety code in this case?

Thanks for your answers!

Answer 1

It is best practice to lock only as few code as possible, because this minimizes chance of lock contention (thus increasing preformance and scalability) and also increases code readability (it is easier to see which objects are protected by lock).

In your case it is sufficient to lock only access to _objects inside Eject method (see code bellow). Locking of method calls return Eject(true) is then not neccessary (but if you do, it will NOT cause deadlock).

Also it is considered BAD practice to use this for locking, because this can lead to deadlocks under certain conditions. So dedicated object is usually stored in private or protected field of enclosing class and used for locking (again see code bellow).

So this should be OK:

class Foo<T>
{
    // Dedicated object instance used for locking
    private Object m_MyLock = new Object();

    T Eject(bool? state)
    {
        lock (m_MyLock)//You typically use the same locking object everywhere within the class
        {
            //somecode
            return _objects[i];
        }
    }

    string SomeOtherMethod()//Some other method that needs to be threadsafe too
    {
        lock (m_MyLock)//You typically use the same locking object everywhere within the class
        {
            return _objects[i].ToString();
        }
    }

    public T TakeNew()
    {
        return Eject(null);
    }

    public T Reserve()
    {
        return Eject(true);
    }
}

---

EDIT:

To clarify OP's comment: You should NOT create new locking object for each lock. You typically use the same locking object within the class. Deadlock situation I was talking about might occur when you use lock(this) in your class and another part of code that is unaware of it (so for example third party library code) uses instance of your class as locking object in such unfortunate way, that results in deadlock. This situation is better explained (including example of such situation) in Why is lock(this) {…} bad?. That's why you should use your private object (m_MyLock) for locking, because then such situation cannot occur (although it doesn't rule out other possible deadlock situations).