2

I'd like to know how you know when to lock an entire type

lock (typeof(MyClass))
{
...
}

or an instance

lock (this)
{
...
}

or an object

lock (this._lockObj)
{
...
}

I am asking this because I have a static class that is a simple Wrapper for Enterprise Library 5 and is accessed by multiple components from possibly different threads. The WriteLog() method is locked. I use the type to lock.

John
  • 3,591
  • 8
  • 44
  • 72
  • 1
    Related to part of this: http://stackoverflow.com/questions/251391/why-is-lockthis-bad – Joe Oct 10 '11 at 12:32

4 Answers4

5

MSDN has this to say: (http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx)

In general, avoid locking on a public type, or instances beyond your code's control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:

lock (this) is a problem if the instance can be accessed publicly.

lock (typeof (MyType)) is a problem if MyType is publicly accessible.

lock("myLock") is a problem because any other code in the process using the same string, will share the same lock.

Best practice is to define a private object to lock on, or a private static object variable to protect data common to all instances.

I have had few cases to ever need anything other than a private member object to lock on.

Joe
  • 41,484
  • 20
  • 104
  • 125
3

First rule. Lock using object that is not accessible from outside of class. That's why this and various typeofs are bad idea. Outer code can interfere with your locks. Then question is what scope of lock is it. If it is static then use static field otherwise use instance field.

To pick correct lock object you should you identify groups of code that are mutually exclusive. So if you have 4 methods that are exclusive in pairs like A and B , C and D you should have 2 differenet lock objects, not 1.

Andrey
  • 59,039
  • 12
  • 119
  • 163
  • 1
    "Inner" code can interfere with your `lock` object, too, which is why I would stretch your recommendation to "use a dedicated locking object, not even used otherwise internally." – Grant Thomas Oct 10 '11 at 12:42
1

In case you want to lock an entire type, you should implement a Singleton pattern.
The responsibility for locking should be inside the actual class implementation - otherwise sooner or later you will run into locking issues - especially deadlocks can only be avoided otherwise when you have a locking order.
Not hiding the locking is a recipe for desaster.

weismat
  • 7,195
  • 3
  • 43
  • 58
-1

The MSDN article suggests locking the type when using a static class/method, so your usage is fine.

It also suggests using 'this' for protecting instance variables.

EDIT

As suggested, here is the MSDN article in question:

http://msdn.microsoft.com/en-us/library/c5kehkcz(v=vs.71).aspx

dougajmcdonald
  • 19,231
  • 12
  • 56
  • 89
  • You might want to link to the MSDN article in question because this doesn't sound right (as whoever gave the minus one is indicating). – Chris Oct 10 '11 at 12:51
  • Yeah looking at things, this was the suggested practice back with VS2k3/.NET 1.1 - I kind of assumed the MSDN article would have defaulted to the most recent iteration...silly me! – dougajmcdonald Oct 10 '11 at 15:49
  • Ah, that explains it. Its quite interesting to see the difference between the 2k3 and 2k5 articles with one recommending something and the next saying don't do it. :) – Chris Oct 10 '11 at 16:45