Question

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.

Was it helpful?

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.

OTHER TIPS

Depends on the case you are facing:

  • If it is possible the first lock is release without touching the second lock it a possible speedup to your application because the second lock won't be locked without being used.
  • If always both locks are used I would prefere the first case for readability

Edit: I think it's not possible to tell this without more information. To keep the locks as short as possible you need to lock as late as possible. Anyway there may be cases you want to get both locks at the same time.

Edit: I said the wrong thing. The first will lock the first lock, then lock the second lock for the portion of the code and then unlock it automatically. The second example will lock the first, then lock the second and then both portions of code has ended, both have unlocked. Personally I would prefer the second option since it will automatically unlock both locks. But as someone else commented, beware of deadlocks. This can happen if another thread locks in the reverse order.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top