Bad practice having 2 locks in same method?

Question

I am wondering if this is a bad code design to have 2 separate locks used in the same method. I believe the reason the two locks were first implemented was the fields they were updating were unrelated.

public class MyClass
{
  private readonly object _lock = new object();
  private readonly object _lock2 = new object();

  public async Task DoStuff()
  {
    lock(_lock)
    {
      //do some stuff
    }
    lock(_lock2)
    {
      //do some other stuff
    }
  }
}

Let's say the _lock is used to only modify an int variable and _lock2 is used to modify and List<string> variable. Both locks can be called from other methods in the class on different threads. Is there a smell for a deadlock here? It seems like I should refactor this so the DoStuff method should only utilize one lock type.

Answer 1

If it's okay for the two blocks of code to be running concurrently from different invocations, then there is no problem with having two objects to lock on. If it's important that the two blocks be able to run at the same time then it's essential that you do have different objects to lock on. If it's important the two blocks not actually run at the same time, and that the entire operation be treated as one logically atomic operation, then you should use one lock.

The two options have entirely different semantics, neither is inherently wrong or inherently right. You need to use the right one for the situation that you're in.