24

I haven't had any issues using the same lock for multiple methods so far, but I'm wondering if the following code might actually have issues (performance?) that I'm not aware of:

private static readonly object lockObj = new object();

public int GetValue1(int index)
{
    lock(lockObj)
    {
        // Collection 1 read and/or write
    }
}

public int GetValue2(int index)
{
    lock(lockObj)
    {
        // Collection 2 read and/or write
    }
}

public int GetValue3(int index)
{
    lock(lockObj)
    {
        // Collection 3 read and/or write
    }
}

The 3 methods and the collections are not related in anyway.

In addition, will it be a problem if this lockObj is also used by a singleton (in Instance property) ?

Edit: To clarify my question on using the same lock object in a Singleton class:

private static readonly object SyncObject = new object();

public static MySingleton Instance
{
    get
    {
        lock (SyncObject)
        {
          if (_instance == null)
          {
              _instance = new MySingleton();
          }
        }
        return _instance;
    }
}

public int MyMethod()
{
      lock (SyncObject)
      {
           // Read or write
      }  
}

Will this cause issues?

Cœur
  • 37,241
  • 25
  • 195
  • 267
alhazen
  • 1,907
  • 3
  • 22
  • 43
  • It is very bad practice to have methods that are named as if they only read things ("GetX") doing writes. Especially if you throw multiple threads into it. – R. Martinho Fernandes Dec 23 '10 at 20:14
  • 1
    It depends on the kind of "write" -- sometimes a lazy read can be a write, and in that case, it's not always bad practice. – user541686 Dec 23 '10 at 20:16
  • Can you clarify that last bit? "In addition, will it be a problem if this lockObj is also used by a singleton (in Instance property) ?" – vcsjones Dec 23 '10 at 20:16

5 Answers5

22

If the methods are unrelated as you state, then use a different lock for each one; otherwise it's inefficient (since there's no reason for different methods to lock on the same object, as they could safely execute concurrently).

Also, it seems that these are instance methods locking on a static object -- was that intended? I have a feeling that's a bug; instance methods should (usually) only lock on instance fields.

Regarding the Singleton design pattern:

While locking can be safe for those, better practice is doing a delayed initialization of a field like this:

private static object sharedInstance;
public static object SharedInstance
{
     get
     {
          if (sharedInstance == null)
              Interlocked.CompareExchange(ref sharedInstance, new object(), null);
          return sharedInstance;
     }
}

This way it's a little bit faster (both because interlocked methods are faster, and because the initialization is delayed), but still thread-safe.

user541686
  • 205,094
  • 128
  • 528
  • 886
10

By using the same object to lock on in all of those methods, you are serializing all access to code in all of the threads.

That is... code running GetValue1() will block other code in a different thread from running GetValue2() until it's done. If you add even more code that locks on the same object instance, you'll end up with effectively a single-threaded application at some point.

Andrew Barber
  • 39,603
  • 20
  • 94
  • 123
6

Shared lock locks other non-related calls

If you use the same lock then locking in one method unnecessarily locks others as well. If they're not related at all than this is a problem since they have to wait for each other. Which they shouldn't.

Bottleneck

This may pose a bottleneck when these methods are frequently called. With separate locks they would run independently, but sharing the same lock it means they must wait for the lock to be released more often as required (actually three times more often).

Robert Koritnik
  • 103,639
  • 52
  • 277
  • 404
2

To create a thread-safe singleton, use this technique.
You don't need a lock.

In general, each lock should be used as little as possible.
The more methods lock on the same thing, the mroe likely you are to end up waiting for it when you don't really need to.

SLaks
  • 868,454
  • 176
  • 1,908
  • 1,964
2

Good question. There are pros and cons of making locks more fine grained vs more coarse grained, with one extreme being a separate lock for each piece of data and the other extreme being one lock for the entire program. As other posts point out, the disadvantage of reusing the same locks is in general you may get less concurrency (though it depends on the case, you may not get less concurrency).

However, the disadvantage of using more locks is in general you make deadlock more likely. There are more ways to get deadlocks the more locks you have involved. For example, acquiring two locks at the same time in separate threads but in the opposite order is a potential deadlock which wouldn't happen if only one lock were involved. Of course sometimes you may fix a deadlock by breaking one lock into two, but usually fewer locks means fewer deadlocks. There's also added code complexity of having more locks.

In general these two factors need to be balanced. It's common to use one lock per class for convenience if it doesn't cause any concurrency issues. In fact, doing so is a design pattern called a monitor.

I would say the best practice is to favor fewer locks for code simplicity's sake and make additional locks if there's a good reason (such as concurrency, or a case where it's more simple or fixes a deadlock).

Jimmy
  • 41
  • 4