48

MSDN gives the following warning about the lock keyword in C#:

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.

Yet it gives no solid reasoning for it. The lock(this) is explained here on SO. I'm interested in lock(typeof(MyType)) case. What is dangerous about it?

Thank you.

Community
  • 1
  • 1
Alex K
  • 10,835
  • 8
  • 29
  • 34

6 Answers6

66

It's dangerous because anything can take that lock so it's difficult or impossible to prevent a deadlock situation.

There used to be an article on this ("Don't Lock Type Objects!" a Dr. GUI article) in with some comments by Rico Mariani. Apparently the article is no longer directly available, but there are 'mirrors' floating around, including at http://bytes.com/topic/c-sharp/answers/249277-dont-lock-type-objects.

Here's an excerpt:

The basic problem here is that you don't own the type object, and you don't know who else could access it. In general, it's a very bad idea to rely on locking an object you didn't create and don't know who else might be accessing. Doing so invites deadlock. The safest way is to only lock private objects.

But wait; it's even worse than all that. As it turns out, type objects are sometimes shared across application domains (but not across processes) in current versions of the .NET runtime. (This is generally okay since they're immutable.) That means that it's possible for ANOTHER APPLICATION running even in a different application domain (but in the same process) to deadlock your application by getting a lock on a type object you want to lock and never releasing it. And it would be easy to get access to that type object because the object has a name—the fully qualified name of the type! Remember that lock/SyncLock blocks (that's a polite word for hangs) until it can obtain a lock. It's obviously really quite bad to rely on a lock that another program or component can lock and cause you to deadlock.

Michael Burr
  • 333,147
  • 50
  • 533
  • 760
  • Though Jon Skeet was first, your reply is more informative and cleared up a confusion I had about the way the lock statement works, so you get a gold star... erm... I mean a green check box :) Thank you. – Alex K Oct 21 '09 at 19:40
  • 2
    Thank goodness for the United States Handicapper General and the 211th, 212th and 213th Amendments that require the Harrison Bergeron-like Jon Skeet to use a molasses-covered keyboard to give the rest of us a fighting chance. – Michael Burr Oct 21 '09 at 19:56
  • 3
    good answer. a common pattern we use in framework code is to have a private instance field just for locking: private object thisLock = new object(); for static locks you can use a private static field: private static object staticLock = new object(); – alexdej Oct 21 '09 at 20:29
28

It's the same problem as with lock(this) - you're locking on a reference which other code has access to, so it could be locking on it too.

If you have two unrelated pieces of code locking on the same reference without intending to exclude each other, then in the best case you could lose a bit of performance due to a lack of concurrency - and in the worst case you could introduce a deadlock.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 2
    Hmm... I think I have a greater misunderstanding of the lock keyword. From your description it seems like if I have 2 locks in completely unrelated parts of my program, which lock on the same type. If one lock is taken by one thread, no thread can enter either this one or the other one? – Alex K Oct 21 '09 at 19:31
  • 3
    Exactly. They're using the same lock, even if they're unrelated bits of code. That is Bad. – Jon Skeet Oct 21 '09 at 19:48
  • On the other hand, there may be times where you want to *allow* external code to sync with your own code. For these, it's probably best to publish the lock object (like all the System.Collection.Synchronized types do by means of the SyncRoot property), but if it is the only object ever locked by your class this is really no different from lock(this). Using only private objects for locking is only really possible if you *don't* ever want to synchronize with external code. – The Dag Jan 09 '12 at 10:23
  • @TheDag: I'd say it *is* different to that, as it would only be done by types which were *explicitly* trying to share a lock which they *knew* you were using. You might "coincidentally" lock on another reference, but you wouldn't "coincidentally" lock on `foo.SyncRoot`. – Jon Skeet Jan 09 '12 at 10:27
  • 1
    @TheDag: Using `SyncRoot` allows code to deal with situations where two or more objects might wrap the same underlying collection, possibly in different ways. For example, a collection which wraps a collection but provides read-only access might have a `SyncRoot` property that returns the underlying collection's `SyncRoot`. Thus, if Proc1 accesses the underlying collection directly while locking its `SyncRoot`, and Proc2 accesses the wrapper while locking on its `SyncRoot`, the accesses by Proc1 and Proc2 would be mutually exclusive. – supercat Jan 16 '12 at 17:50
2

Because the result of typeof (MyType) (which is an object of type Type) is widely accessible and other thread can lock on the same object, and hold that lock indefinitely. Then internal logic of MyType has effectively given away significant control over it's synchronization logic. This might not be an actual problem if this is intended, but coding defensively/skeptically should be your modus operandi.

G-Wiz
  • 7,370
  • 1
  • 36
  • 47
1

This wouldn't be "an issue" if this modified-parallel form of the advice were followed:

In general, avoid locking on a public type, or instances that you did not create or define. The common constructs lock (this), lock (typeof (MyType)) violate this guideline if you did not create the instance or declare the type..

However, as the above 'cannot be guaranteed' for public types or accessible instances across all encountered code, the MSDN and other sources argue that these should be avoided for Defensive Programming against a singular potential hard-to-detect runtime (Deadlock) issue. This is good advice given that most coders are not very good or diligent about rules..

..and someone who encountered such bug in the wild would be much more adamant about not allowing this specific issue to occur again, by imposing the guidelines stated. (Java 1.0/1.1 with the Threaded AWT UI Model was especially problematic.)

The case of lock ("mylock") is singularly special in that it should be avoided due to string-interning as one generally cannot "know" if they are violating the advice above..

user2864740
  • 60,010
  • 15
  • 145
  • 220
0

because the target of a lock is ONLY to establish a place to store the lock boolean (Am I locked or not) for other threads to look at....

The common misconception that the target of a lock is actually somehow being locked is just wrong... What is "locked" is, .... nothing, unless in methods which can access some shared memory in an unsafe manner, you write the code to look at the this lock and not proceed until it has been released... using a Type object, as a lock target is wrong because code snippets anywhere in the entire solution process space can access that type object and change the synch block that the lock boolean is stored in. Creating a locally scoped object allows you to better ensurethat only those threads and methods that can access or mess with your "at risk" shared memory can also access and/or modify the lock.

Charles Bretana
  • 143,358
  • 22
  • 150
  • 216
0

It is stated also documentation under topic "Managed Threading Best Practices". https://msdn.microsoft.com/en-us/library/1c9txz50(v=vs.110).aspx

It says;

Don't use types as lock objects. That is, avoid code such as lock(typeof(X)) in C# or SyncLock(GetType(X)) in Visual Basic, or the use of Monitor.Enter with Type objects. For a given type, there is only one instance of System.Type per application domain. If the type you take a lock on is public, code other than your own can take locks on it, leading to deadlocks. For additional issues, see Reliability Best Practices.

Use caution when locking on instances, for example lock(this) in C# or SyncLock(Me) in Visual Basic. If other code in your application, external to the type, takes a lock on the object, deadlocks could occur.

Teoman shipahi
  • 47,454
  • 15
  • 134
  • 158