Search code examples
c#lockingcoverity

Double locking specific case C#


I struggle to choose from this 2 approaches, many of the answers here favor one or the other. I need a guidance to choose the best for my situation.

lock (lockObject)
lock (lockObject2) {
     // two critical portions of code
}

versus:

lock (lockObject)
{
    //first critical portion for lockObject 
    lock (lockObject2) {
       //critical portion for lockObject2 and lockObject
    }
}

The second example is marked as a bad practice by Coverity and I want to switch to the first if it is OK to do that.

Which of the 2 is the best(and by best I mean code quality and fewer problems on the long run)? And why?

Edit 1: The first lock is only used for this case in particular.


Solution

  • "Best" is subjective and depends on context. One problem with either is that you can risk deadlocks if some code uses the same locks in a different order. If you have nested locks, then personally at a minimum I'd be using a lock-with-timeout (and raising an exception) - an exception is better than a deadlock. The advantage of taking out both locks immediately is that you know you can get them, before you start doing the work. The advantage of not doing that is that you reduce the time the lock on lockObject2 is held.

    Personally, I would be looking for ways to make it:

    lock (lockObject) {
        //critical portion for lockObject
    }
    lock (lockObject2) {
        //critical portion for lockObject2
    }
    

    This has the advantages of both, without the disadvantages of either - if you can restructure the code to do it.