9

I have a class with a field of type collection.

Questions:

  1. if I lock(this), do I effectively lock the collection too?
  2. what is more efficient, to do lock(this) or to create a SyncRoot object and do lock(SyncRoot)?
Dolbz
  • 2,078
  • 1
  • 16
  • 25
Nestor
  • 13,706
  • 11
  • 78
  • 119

5 Answers5

12

Don't lock on this. It could be the case that someone else has used the instance as a lock object too. Use specifically designated lock objects.

1) if I lock(this), do I effectively lock the collection too?

No.

2) what is more efficient, to do lock(this) or to create a SyncRoot object and do lock(SyncRoot) ?

Efficient? Focus on semantics. locking on this is dangerous. Don't do it. The difference in performance, if any, is not material.

Seriously, it's akin to asking, what will get me to my destination faster, driving 100 MPH the wrong way down the freeway, or walking?

jason
  • 236,483
  • 35
  • 423
  • 525
  • 1
    Thanks Jason. Great answer. The "efficiency" part of my question was because I thought that lock(this) went ahead and locked all the fields inside the class too. I understand now that's not the case. – Nestor Jun 28 '11 at 17:14
  • 2
    @Nestor: Oh no, it definitely doesn't do that. Basically, `lock` just adds a reference to the instance to some collection (maybe it's a `HashSet`, who knows, doesn't matter). When someone else tries to `lock` on the same instance, the `lock` mechanism checks to see if this reference is already in the collection. If it is, it blocks. If it is not, it adds it to the collection and acquires the `lock` returning to the caller. When the caller is done, the reference is removed from the collection. That is it. It doesn't make the `object` read only, or anything like that. – jason Jun 28 '11 at 17:16
  • `lock(this)` may "lock the collection" (in the sense of locking `SyncRoot`) depending on how SyncRoot is implemented. But of course you shouldn't rely on this. And `lock(this)` and `lock(SyncRoot)` are both equally "dangerous". See my answer. – Joe Jun 28 '11 at 17:23
  • I think you missed that he said "__create__ a `SyncRoot` object and do `lock(SyncRoot)`" and is not referring to an already existing property on some types/interfaces in .NET. – jason Jun 28 '11 at 17:35
8

Always use lock(_syncRoot).

Where _syncRoot is a private field (just has to be an object).

This is no difference in terms of efficiency, but you're better to have a private field that you're in control of to lock on. If you lock on this, another object may also be locking on it.

See Why is lock(this) {...} bad? for a much better explanation. Also have a look at the msdn article on lock.

By locking on a collection, you aren't doing anything to stop it from being changed. A misunderstanding you might have, is that lock doesn't do anything special to stop that object being changed, it only works if every critical piece of code also calls lock.

Community
  • 1
  • 1
Ray
  • 45,695
  • 27
  • 126
  • 169
  • 1
    There *may* be a good reason for `lock(this)` but ... generally good advice. –  Jun 28 '11 at 17:10
3

When using lock you aren't doing anything magical to the object you put inside the lock - it isn't making it read only or anything like that. It is just making a note that something has a lock reference to that object. So if anybody else tries to get a lock on that object at the same time it will do what you expect (prevent synchronous access).

What lock doesn't do is care about any properties, fields or anything else in the object you are locking. So no, you aren't locking the collection at all.

This is explained at greater length in this question: Why is lock(this) {...} bad? (which I got from other answers but is an excellent answerand I felt it should be included here too).

As for efficiency I wouldn't expect there to be a lot of performance difference between the two. However as others have said you should not lock on something that might be locked by something outside of your control. This is why more often than not you will find private variables being created for this.

Personally I'd give it a more descriptive name than synclock to describe exactly what the locking process is for (eg saveLock).

Community
  • 1
  • 1
Chris
  • 27,210
  • 6
  • 71
  • 92
  • I really like how this answers talks about how `lock(obj)` actually relates to `obj`. Locks in C# are *advisory* in that all parties wishing to enter must lock upon the same object to ensure exclusive access to the critical region. –  Jun 28 '11 at 17:13
  • @pst: Cor, that was some shocking language you had to clear up for me there. Thanks. My typing is always worst at the end of the day. :) – Chris Jun 28 '11 at 17:14
2

A lot of people are saying that lock(this) is dangerous. However The MSDN description of ICollection.SyncRoot states:

For collections whose underlying store is not publicly available, the expected implementation is to return the current instance

If a class follows this guideline, then yes, lock(this) is effectively the same as lock(SyncRoot). But you shouldn't rely on implementation details like this, and should use the more explicit lock(SyncRoot).

Of course, if you don't want to publicly expose locking semantics, but use lock within your class implementation, then you should lock on a private object as others have recommended, and as MSDN recommends. But that doesn't seem to be what you're asking.

Both locking on an instance (lock(this)) and locking on a public property (lock(SyncRoot)) expose you to having the lock taken by code you don't control. If your intention is to expose locking semantics to callers, you have no choice but to do this.

Joe
  • 122,218
  • 32
  • 205
  • 338
  • 1
    1. I think you missed the OP says "to create a `SyncRoot` object and do `lock(SyncRoot)`." That is, he is not referring to `ICollection.SyncRoot`. 2. It's been long known that `ICollection.SyncRoot` was a [mistake](http://blogs.msdn.com/b/brada/archive/2003/09/28/50391.aspx). 3. It's even stated in the [MSDN documentation](http://bit.ly/iVv9ry) on `lock` that `lock`ing on `this` can be a mistake (if the instance is public). 4. You shouldn't expose a `lock` object. – jason Jun 28 '11 at 17:34
  • @Jason, I agree with most of what you're saying. But for your first point, the only reason I would create a `SyncRoot` property is in a class that implements `ICollection`. This is what I assumed the OP meant when he talked about collections. And while `ICollection.SyncRoot` was certainly a mistake, there are still cases (e.g. COM Interop, which doesn't support generics) where you may want to implement it, and in such cases you are stuck with it. – Joe Jun 28 '11 at 20:32
1

Always lock on something over which the locking code has control. You have no control over a type or an instance of a type.

kprobst
  • 16,165
  • 5
  • 32
  • 53