c# lock on reference passed to method - bad practice?

Question

I have a method similar to:

public static void DoSomething (string param1, string param2, SomeObject o) 
{
   //.....

   lock(o) 
   {
       o.Things.Add(param1);
       o.Update();
       // etc....
   }
}

A few points:

1. Is locking in this way bad practice?
2. Should I lock on a private static object instead?
3. If so, why?

Answer 1

To minimize side effects, the object being locked on should not be the object being manipulated but rather a separate object designated for locking.

Depending on your requirements, there are a few options for handling this issue:

Variant A: Private locking object

Choose this if you just want to ensure that DoSomething does not conflict with a parallel instance of DoSomething.

private static readonly object doSomethingLock = new object();

public static void DoSomething (string param1, string param2, SomeObject o) 
{
   //.....

   lock(doSomethingLock) 
   {
       o.Things.Add(param1);
       o.Update();
       // etc....
   }
}

Variant B: Pass locking object as a parameter

Choose this if access to o must be thread-safe even outside of DoSomething, i.e., if the possibility exists that someone else writes a method DoSomethingElse which runs in parallel to DoSomething and which must not interfere with the lock block in DoSomething:

public static void DoSomething (string param1, string param2, SomeObject o, object someObjectLock) 
{
   //.....

   lock(someObjectLock) 
   {
       o.Things.Add(param1);
       o.Update();
       // etc....
   }
}

Variant C: Create SyncRoot property

If you have control over the implementation of SomeObject, it might be convenient to provide the locking object as a property. That way, you can implement Variant B without having to pass around a second parameter:

class SomeObject
{
    private readonly object syncRoot = new object();

    public object SyncRoot { get { return syncRoot; } }

    ...
}

Then, you just use lock(o.SyncRoot) in DoSomething. That's the pattern some of the BCL classes use, e.g., Array.SyncRoot, ICollection.SyncRoot.

Answer 2

Just answering your 3rd question:

Imagine that latter on you decide to lock on another method parameter, maybe something like:

public void XXX(object o)
{
    lock(o)
    {

    }
}

You will have a hard time trying to see if there is a deadlock. You will need to check that the object passed as parameter to SomeObject o is never passed as parameter to object o at the same time.