2

We have a code with two locks locking on same object in different methods. Both methods can be called from different threads.

Can this go into deadlock scenario ? Should we use same lock1 in both methods ?

Reason for using two different locks is that removal happens from various tasks running in parallel but updating the list happens from new thread running every few seconds.

private static Object Lock1 = new Object();
private static Object Lock2 = new Object();
private static List<string> list = new List<string>();

public void Method1()
{
  lock(Lock1)
  {
   //update list
  }
}

public void Method2()
{
  lock(Lock2)
  {
    //remove from list
  }   
}

Update

Thanks for healthy discussion, I have updated my code to use thread safe collection BlockingCollection provided by .net. I continue to use the lock1 as I need to control how many number of objects the list can contain. I have removed the lock2 now as its not needed with the thread safe list.

Zeus
  • 2,602
  • 6
  • 41
  • 54
  • Deadlock most often occurs with nested locks. However you usually want one lock to protect mutating the same object, in your case the list. – juharr Sep 18 '17 at 18:23
  • 4
    You need a deeper understanding of threading issues before you should write multi-threaded code. You're completely misunderstanding the point of locks. – SLaks Sep 18 '17 at 18:25
  • current code is not going to protect list you need to use same object to synchronized access to list... – sumeet kumar Sep 18 '17 at 18:26
  • @SLaks yes I am learning as I go, unfortunately I have been given someone else's code to maintain. – Zeus Sep 18 '17 at 18:27
  • I see so if method 1 waits 1 min to release the lock, method 2 will still go in and try to update the list. This can be a problem as I may not see deadlock but the list is not going to be protected correct ? – Zeus Sep 18 '17 at 18:29
  • 1
    "Learn as you go" is often a good idea, but multithreading is complex, difficult, and requires a *thorough* understanding of a great many issues to get correct. You *will* make terrible undetectable mistakes that leave your program fragile and hard to fix. Rather than learning as you go, consider studying books, taking a course, or finding a mentor who can help you with this. – Eric Lippert Sep 18 '17 at 22:14

2 Answers2

6

Can this go into deadlock scenario?

No.

Should we use same lock1 in both methods ?

Yes. You should always lock on the same lock object when accessing a particular object on multiple threads. In your specific case the code you've shown is completely wrong and broken. You should have one lock per list, and consistently take out that lock every time you access the list.

If you are updating the list variable then same thing -- you should have one lock for the list variable, and every single time you access the variable for any reason needs to be under that lock.

Reason for using two different locks is that removal happens from various tasks running in parallel but updating the list happens from new thread running every few seconds.

That doesn't matter. All updates, whether they are removals or otherwise, must happen under the same lock.

If you are in a situation where you have many frequent readers and few writers, there are special-purpose locks for those cases. But that is not your scenario.

Some questions you did not ask:

What causes a deadlock?

void Transfer(Account a1, Account a2, decimal amount)
{
  lock(a1)
  {
    lock(a2)
    {
      // do the transfer
    }
  }
}

Suppose thread X calls Transfer(savings, checking, 100) and gets as far as locking savings. Then we switch to thread Y which calls Transfer(checking, savings, 50). We lock checking, and then attempt to lock savings, but cannot, because X has it. We then switch back to X, which tries to lock checking, but cannot, because Y has it. We then wait forever. That's a deadlock.

Does locking the same object "nested" on the same thread cause a deadlock?

No. The answer which says that is wrong. Taking a lock you already have on the thread automatically succeeds.

Are there better techniques I should be using?

Yes. Multithreaded programs are hard to write correctly. If you must, use high-level objects such as multithreaded collections or immutable collections that are designed to solve these problems efficiently without explicit locks.

You should also read

Confusion about the lock statement in C#

Eric Lippert
  • 612,321
  • 166
  • 1,175
  • 2,033
  • Thanks Eric, yes its hard indeed. My current scenario is that I do not want removal from collection to wait around if updating code is taking long time. While updating collection is happening, I want removal to continue. This way I am ok if the entire list is empty before the update happens. Its a guarantee that there will be no duplicates – Zeus Sep 18 '17 at 23:30
  • What do you want to happen if there is adding happening, that the removal happens later, or that it just doesn't happen? – Jon Hanna Sep 19 '17 at 11:15
  • 5
    @Zeus. Your current scenario is that you're waiting at a stoplight for the light to turn green, and you've apparently decided that you're waiting too long and you're just going to pay attention to a **different stoplight for a different intersection**, because you want to drive fast. Does this strike you as a good idea? It does not strike me as a good idea. – Eric Lippert Sep 19 '17 at 11:17
  • @Zeus: There are *rules* in multithreading and they exist for *reasons*. You can't just ignore them because you don't want the performance penalty of lock contention. That performance penalty is *the price you pay for sharing resources on multiple threads*, the same way that waiting at traffic lights sometimes is the price you pay for sharing the road with other people. – Eric Lippert Sep 19 '17 at 11:19
3

That won't deadlock, unless there's something calling from the commented out portion into the other method and so potentially leading to the case where one thread has the first lock and is waiting for the second, while another has the second and is waiting for the first.

The big problem here is the locks are not protecting the list. The reason you need the lock in the first place is that List<T> wasn't designed for concurrent use, so you need to serialise access. Since the Add and Remove methods both involve copying elements within arrays, maintaining counts and swapping one internal for another on growing past its capacity there are plenty of opportunities for a simultaneous add and remove to fail to add, fail to remove, mess up the internal count and either have a mysteriously added null or something removed that shouldn't be. More generally there's not even a guarantee that it won't be put into a state its other code assumes is impossible and result in weird errors later on.

You need to protect the list from either method in both methods, so they need to use the same lock.

Jon Hanna
  • 102,999
  • 9
  • 134
  • 232
  • Understood however I need the updating part of the list to be protected. Code that removes from list should not wait until the list is updated. This is why 2 locks. Thoughts ? – Zeus Sep 18 '17 at 19:56
  • 1
    @Zeus Do you consider it important for your code to actually work, or do you consider it okay for your removal to do one of the following: 1. throw an exception instead of removing the item 2. corrupt the list such that it becomes unusable and has nonsensical data in it 3. fail to remove the item from the list, but not throw any exceptions 4. remove the item from the list for a little while, but then have it show up again later 5. any number of other things I could think of off of the top of my head. 6. you get lucky and it just works. Keep in mind it won't constantly be one of those either. – Servy Sep 18 '17 at 20:02
  • @Zeus Of you're okay with one of those random options happening each time you run the code, then your option is fine. – Servy Sep 18 '17 at 20:02
  • If you don't want to wait then an approach that can work is to have a queue of pending operations you add to and let it go on, but then you need to serialise access to that queue the same way, so no gain here. If there was something else happening as well as the removal then maybe it'd be worth it. – Jon Hanna Sep 18 '17 at 22:34
  • Or it could be perhaps a case for the producer consumer pattern (the example code isn't, but code where you do something with the removed value could be). But in terms of the locks, step back: Why do you have locks at all? What are they there to protect against? Do they succeed? That's how to look at this. – Jon Hanna Sep 18 '17 at 22:38
  • @servy the list I am using is thread safe so it would not have problem if I am removing one item and another item is being added. Its guarantee that there will not be any duplicate – Zeus Sep 18 '17 at 23:23
  • The code in the question uses `List` which is not threadsafe. – Jon Hanna Sep 19 '17 at 08:45
  • @Zeus There is no such collection in the .NET library, so you are apparently using some 3rd party product or collection you've written yourself, if that's the case. What 3rd party product is it that you're using that is giving you a thread safe list? – Servy Sep 19 '17 at 13:10
  • And if it is threadsafe, then what is the purpose of the locks at all? I can think of a few reasons why one might want locks at such points, and for some of them two separate locks would make sense, but for others they wouldn't. – Jon Hanna Sep 19 '17 at 13:15
  • Thanks for all the replies, its been very useful, I have updated the code to use BlockingCollection now which is threadsafe list. I still have to lock it for concurrency as I have to maintain the count of objects within the list – Zeus Sep 19 '17 at 22:05
  • If the backing of the `BlockingCollection` has a `Count` then you don't need to lock on that either. – Jon Hanna Sep 20 '17 at 09:05