views:

155

answers:

4

Hi, Altough I am using locks in my app, I do not understand what exactly does locking on particular reference type. I thought it just stops the thread until the content of {} is finished. But I have read that locking(this) is bad, if its public - why? The article explained it but I do not understand sice I do not know what happened to the object itself being locked. For example, what if I use lock(this) and from another thread call its method? I thought only the code under the lock is protected, or I will be unable to access the locked object at all? Thanks

+6  A: 

Each object on the managed heap can be used as a lock-object, which is a means to synchronize access between threads.

I thought it just stops the thread until the content of {} is finished.

Well, a lock it stops other threads from acquiring the lock until the lock is released, which is most commonly at the end of a lock statement (but it could also be a Monitor.Wait).

The lock(this) usage is dangerous because locking is complex, and understanding exactly which threads are locking which object(s) at which time is very important to avoid deadlocks; however, if you lock(this) you aren't in control of other threads - which could also be (unexpectedly) locking the same object. It is far safer to use a private field for locking.

As an example, if you have (in a synchronized list):

private IList<T> innerList = ...
public int Count { get { lock(this) { return innerList.Count; } } }

Then it isn't hard to imagine another bit of code also having a reference to this synchronized list, and locking on it, for example:

SyncList<T> list = ...
lock(list) { // lock for atomicity
    if(!list.Contains(value)) list.Add(value);
}

Which is a potential deadlock; it would be much better if Count didn't lock(this), but locked a private object, i.e.

private readonly object syncLock = new object();
public int Count { get { lock(syncLock) { return innerList.Count; } } }

Now there is no risk of this issue. Another issue here is that both field-like events and [MethodImpl] cause lock(this). Locking on a Type (for static methods) is equally dangerous for exactly the same reasons.

Marc Gravell
+3  A: 

The object itself isn't locked. Think of each object as having an associated lock (or monitor). When one thread has acquired the lock, no other thread can acquire it without the first thread releasing it, either by calling Monitor.Exit (which is what happens at the end of a lock statement) or by calling Monitor.Wait. A thread calling Monitor.Enter to acquire the lock will block until it can acquire the lock.

The object itself isn't "protected" at all - locks are advisory, basically.

The reason for not locking on "this" is that you don't know what other code has a reference to "this". Getting locking right requires you to know in which situations a thread will or won't own the lock - and you can't know that if code outside your control can take out the lock. The exception to this is if you're exposing a reference for the explicit purpose of sharing a lock (like the SyncRoot property on .NET 1.1 collections).

Jon Skeet
A: 

Locking an object does nothing to the object itself - it simply means that any other thread trying to lock the same object will be stalled until the locking thread releases it. The reason that lock(this) is frowned upon is that other code outside the object can also lock the object and thus interfere with its normal operation.

On Freund
A: 

When you use lock(someObject) { ...code... } it means only when thread can be inside the locked region at a time (assuming you always lock on the same object!). A second thread that attempts to enter the region will block and have to wait until the first thread leaves. If you have two lock (someObject) { } regions locking on the same object, only one thread can enter any of those regions.

Locking is usually an implementation detail that is to be kept private. It is a bad idea to lock on a public member because it means that clients of your class can also lock on this same object and it can cause deadlocks or unnecessary slowdown. Locking on this is bad for the same reason.

Create a private new object and lock on that.

Mark Byers