CODEWITHSUNDEEP
c#multithreadinglocking
Yeonho
Yeonho

Reputation: 3623

rationale behind lock inside lock?

I am reviewing an example code in a book and came across the following code(simplified). In the code, when Subscribe(T subscriber) is called, the thread enters into a lock section. and then, when code inside the lock calls AddToSubscribers(T subscriber) method, the method has another lock. why is this second lock necessary?

public abstract class SubscriptionManager<T> where T : class 
{
   private static List<T> subscribers;
   private static void AddToSubscribers(T subscriber)
   {
      lock (typeof(SubscriptionManager<T>))
      {
         if (subscribers.Contains(subscriber))
            return;
         subscribers.Add(subscriber);
      }
   }

   public void Subscribe(T subscriber)
   {
      lock (typeof(SubscriptionManager<T>))
      {
         AddToSubscribers(subscriber);
      }
   }
}

Upvotes: 5

Views: 373

Answers (4)

Shamim Hafiz - MSFT
Shamim Hafiz - MSFT

Reputation: 22094

I also faced a situation once where I had to use nested Lock.

My case was, the function of the second lock maybe called from elsewhere, since it was a static function. However, for your case it won't be necessary since each data member belongs to an Instance and not static..

Upvotes: 1

Alexei Levenkov
Alexei Levenkov

Reputation: 100547

You probably should read near that sample and see what book talks about.

For that particular case - no, second lock is unnecessary.

Note: The sample is dangerous since it locks on public object (type). Normally one locks on special private object so external code is not able to mistakenly introduce deadlocks by mistakenly locking on the same object.

Upvotes: 1

to StackOverflow
to StackOverflow

Reputation: 124696

In the code you posted, it isn't necessary. But then again, the code you posted is incomplete - for example the subscribers list is never initialized.

Locking on typeof(SubscriptionManager) is probably not a good idea either - locking on the subscribers field would be better - but would require the subscribers field to be initialized, e.g.

private static List<T> subscribers = new List<T>();

Upvotes: 3

Marc Gravell
Marc Gravell

Reputation: 1062770

In that context, it isn't; however, since locks are re-entrant that can be useful to ensure that any other caller of AddToSubscribers observes the lock. Actually, for that reason I'd say "remove it from Subscribe and just let AddToSubscribers do the locking".

However! A lock on a Type is pretty dangerous. A field would be safer:

// assuming static is correct
private static readonly object syncLock = new object();

and lock(syncLock). Depending on when subscribers is assigned, you might also get away with lock(subscribers) (and no extra field).

I should also note that having an instance method add to static state is pretty.... unusual; IMO Subscribe should be a static method, since it has nothing to do with the current instance.

Upvotes: 9

Related Questions