Is this lock usage thread safe?

Question

I know that is wrong to use lock(this) or any shared object.

I wonder if this usage is OK?

public class A
{
  private readonly object locker = new object();
  private List<int> myList;
  public A()
  {
    myList = new List<int>()
  }

  private void MethodeA()
  {
    lock(locker)
    {
      myList.Add(10);
    }
  }

  public void MethodeB()
  {
    CallToMethodInOtherClass(myList);
  }
}

public class OtherClass
{
  private readonly object locker = new object();
  public CallToMethodInOtherClass(List<int> list)
  {
   lock(locker)
   {
     int i = list.Count;
   }
  }
}

Is this thread safe? In OtherClass we lock with a private object so if the class A lock with its private lock can the list still change in the the lock block in OtherClass?

Solution 1

No, it's not thread safe. Add and Count may be executed at the "same" time. You have two different lock objects.

Always lock your own lock object when passing the list:

  public void MethodeB()
  {
    lock(locker)
    {
      CallToMethodInOtherClass(myList);
    }
  }

Solution 2

No this is not thread safe. To make it thread safe you can use lock on static objects because they are shared between threads, this may cause deadlocks in the code but it can be handle by maintaining proper order for locking. There is a performance cost associated with lock so use it wisely.

Hope this helps

Solution 3

No, this is not thread-safe. A.MethodeA and OtherClass.CallToMethodInOtherClass are locking on different objects, so they're not mutually exclusive. If you need to protect the access to the list, don't pass it to external code, keep it private.

Solution 4

No, that is not thread-safe.

Your 2 methods are locking on 2 different objects, they will not lock out each other.

Because CallToMethodInOtherClass() only retrieves the value of Count nothing will go horribly wrong. But the lock() around it is useless and misleading.

If the method would make changes in the list you would have a nasty problem. To solve it, change MethodeB:

  public void MethodeB()
  {
    lock(locker)  // same instance as MethodA is using
    {
      CallToMethodInOtherClass(myList);
    }
  }

Solution 5

No, they have to lock the same object. With your code they both lock on a different and each call could be executed simultaneous.

To make the code thread safe place a lock in MethodeB or use the list itself as lock object.

View more solutions

Comments

[Maya]

but i thought its wront to lock with shraed object

[Amar Palsapure]

Static objects are shared among other object, so if you put lock on this, it will be consume by only one object at one time. Read more about static on MSDN or lock on MSDN (Best practice is to define a private object to lock on, or a private static object variable to protect data common to all instances. MSDN)

[Maya]

@ Felix I thoought that its wrong to lock with shared object so lock with the list itself its wrong?

[Amar Palsapure]

Great... Use lock whenever it is absolutely necessary, there is performance impact when using it.

[Jon Hanna]

This is less thread-safe. Aside from the locking object being broader than necessary you've exposed the locking object so it's now possible for different outside code to use it in different ways alongside other locking objects making deadlock possible.

[Amar Palsapure]

+1 That's right, I was explaining the use of static object in case of lock, ideally all the lock object should have limited scope.

[Maya]

@Amar but shared objects can make deadlocks. see here

[Amar Palsapure]

Yes right, that's why I mentioned it should be use wisely. It's upto developer how to use lock for synchronization. If you don't want to do synchronization yourself, you can use data types such Hashtable which are threadsafe.

[Maya]

@AmarPalsapure ok thanks for your answer and for the comments

[Jon Hanna]

It actually is thread-safe, though only as an implementation detail of Count being thread-safe which isn't promised, and not in a way that would extend well into more realistic code.

[Felix K.]

@Maya That's true, it's wrong. But it's a easy way to solve the problem described. But anyway the right way would be to create a thread-safe list or a container which contains the list and access the list only over the container.

[Eric Lippert]

@AmarPalsapure: The performance cost of an uncontended lock is 20 to 80 nanoseconds. Uncontended locks are usually not a performance problem.

[Eric Lippert]

@AmarPalsapure: hash tables are not threadsafe in general. Some may be threadsafe for readers only scenarios; consult the documentation for details.

[Eric Lippert]

@Maya: shared objects do not cause deadlocks. Improper lock ordering causes deadlocks.

[Jon Hanna]

@AmarPalsapure Hashtable is thread-safe for a single writing thread and many readers. While that's not an unheard-of combination, its not unheard-of to have multiple writers to a dictionary-style collection like hashtable either. And even then, as I point out in my answer, combinations of thread-safe operations can easily combine to no longer be thread-safe.

[Jon Hanna]

@EricLippert it is true though that the smaller the set of code to which a lock is visible, the easier it to avoid improper ordering. The exposure of LockObject.Locker above makes it even possible for two pieces of code in different assemblies to acquire it and another lock, in a different order. With private lock objects its much easier to analyse the code to be sure that that never happens.

[Pål Thingbø]

Use ConcurrentBag or ConcurrentDictionary instead, then you won't have to worry about locking.

[Jon Hanna]

@PålThingbø Not true in the slightest. First while those collections, and indeed my own thread-safe dictionary and set and some other such collections, are thread-safe for each individual action on them, groups of actions on them are not necessarily thread-safe for the reasons I give above (think about it, int is thread-safe, but that doesn't make ++x thread-safe, because the three thread-safe actions that entails are not thread-safe as a unit). There's also rarely much point suggesting someone use a bag or dictionary when they want a list; the...

[Jon Hanna]

@PålThingbø semantics of those classes are completely different. I will hopefully be releasing a thread-safe list class soon that gives much better concurrent behaviour than that in the answer above, though it will still be limited (no RemoveAt or Insert; I might produce a different class that has them too later, but doing them thread-safe without locking is much more cumbersome than the rest), and it still won't make code using it magically thread-safe as a whole, any more than ConcurrentDictionary does, again for the reasons I give above.

[Pål Thingbø]

Thanks @Jon for the clarification. I was giving advise based on the code in question, which is simply adding and removing items to a collection. Concurrent is a great tool for this.

[Jon Hanna]

@PålThingbø since the code in the question just adds the value 10, the best complete replacement is one that just keeps count of how many 10s were added! In terms of real code, I suppose your guess that they don't need list semantics is as good as my guess that they do. Still, concurrent objects do still need care to make sure sets of operations one does with them are still thread-safe as a whole.

[Pål Thingbø]

I agree. I guessed that he just wanted the collection, and that List.Count was what he was looking for. Anyway, love your code, and keep up the good work.

[Jon Hanna]

@PålThingbø ConcurrentBag.Count can be one thing one gets most tripped up on with careless use. Thanks for the compliment; that project has been quiet lately, but will be busy for a while as I'm doing a refactor and then will finally mark it "1.0" soon (after which I'll be a lot stricter about breaking changes). Do feel free to suggest changes (patches or pull-requests).

[Fabricio]

If the locking object is private static it won't be visible externally.

[Fabricio]

You should avoid having locking objects exposed as public. If your intention is solely the thread synchronization, the locking object should be private.