7

I'm trying to understand the point of the syncroot in ICollection. Why not just lock the collection?

lock(myCollection)
{
    //do stuff to myCollection
}

vs

lock(myCollection.SyncRoot)
{
    //do stuff to myCollection
}
Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
richard
  • 12,263
  • 23
  • 95
  • 151

2 Answers2

8

Typically, if thread safety is a serious concern, I would avoid either of these options.

A far better option is typically to maintain your own private variable, and lock on it in all methods where it's required - including the entire public API that accesses the collection.

The real danger is that, by locking on a type that's exposed or could be exposed to the outside world, you potentially open up the ability for the "outside world" to mess with your synchronization. If more than one lock is being used, this can lead to dead locks (if the outside locks on something you aren't expecting).

By creating a private variable, and locking exclusively on it, you're "taking control" of the situation. This makes it much more clear what is occurring. In addition, it eases synchronization between multiple objects, especially later as you maintain the code, since the lock is very clear.

Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
  • @Reed Copsey: I'm confused. Doesn't the object being locked have to be exposed in the same way as your collection, i.e. if two separate pieces of code lock their own private variable in order to lock a collection they both have access to, how are they preventing each other from accessing the collection at the same exact time? Don't you want all code (no matter where it's from) calling the collection to use the same object to lock the collection? – richard Jun 29 '11 at 18:09
  • 2
    You should expose neither the collection nor the lock. – CodesInChaos Jun 29 '11 at 18:10
  • @CodeInChaos: What's the purpose of a collection if you can't use it? – richard Jun 29 '11 at 18:12
  • 1
    You use it privately, implement whatever operations you need with the appropriate thread safety on it and expose only safe operations to the outside. – CodesInChaos Jun 29 '11 at 18:12
  • @Richard: Typically, I recommend NOT exposing the collection directly, but wrap it inside of your class. Expose the appropriate interface(s). All of the locking can then happen inside of your property get/set accessors as needed, and be done on a private object. The outside world never sees your lock, and never has to worry about it, since you're basically becoming thread safe (as far as usage is concerned) – Reed Copsey Jun 29 '11 at 18:24
  • 1
    @Reed Copsey: Ok, sorry to beat a dead horse here, but I just realized my confusion. If your collection isn't exposed, then what's the big deal in locking with it? You say "The real danger is that, by locking on a type that's exposed or could be exposed to the outside world, you potentially open up the ability for the "outside world" to mess with your synchronization." But if your collection is exposed, you can't use a private variable (because then your not really locking). If it isn't exposed, it seems there is no difference in using a private variable or the collection as your lock object. – richard Jun 29 '11 at 18:30
  • 1
    @Richard: There isn't - provided you only use a single object and always use that object for locking. By using a separate private object, you're making it much more clear. This becomes more important when you're dealing with multiple items that all need synchronization - a separate "lockObject" makes it more clear == more maintainable over time. However, you can still just lock on the collection, if you want to use it... – Reed Copsey Jun 29 '11 at 18:31
  • @Reed Copsey: Ok, that makes sense. This is one of those "better practices gained through experience" things. I will take the advice. Thank you again. – richard Jun 29 '11 at 18:33
  • @Richard: Yeah - it's more of a "best practice" thing than a technical thing. If you see a class with this at the top: `private object lockObj = new object();` you kind of know, right away, that there is some threading issues in place, and as you maintain it, you know where to look for the synchronization and how to go about making sure it stays thread safe.... – Reed Copsey Jun 29 '11 at 18:41
  • @Richard: To add a bit more: Unfortunately the `lock(this)` pattern is still quite common. So the rule is: Whenever you use a complex object where you are not in full control of the impementation details don't lock on it as it might use `lock(this)` which can lead to hard to find dead locks. – ChrisWue Jul 11 '11 at 22:14
1

Never lock on SyncRoot because I believe it does a lock(this) which is locking on the entire collection. If you're going to lock, you should create an object and lock on it. For example

public class MyClass
{
   get {return lock(_lockObject) _myCollection ; }
   set {lock(_lockObject) _myCollection.Add(value); }


   private List<string> _myCollection = new List<string>();
   private object _lockObject = new object();
}

To better clarify, lock(myCollection.SyncRoot) and lock(myCollection) do the same thing.
Because the property SyncRoot looks something like this

get { return this; }

ChrisWue
  • 18,612
  • 4
  • 58
  • 83
null_pointer
  • 1,779
  • 4
  • 19
  • 38
  • Michael: Very interesting. The `SyncRoot` _is_ returning the collection._ lol Then what is the point of even having the `SyncRoot` property? – richard Jun 29 '11 at 18:13
  • 1
    It appears I'm wrong. Here is the code for SyncRoot: object ICollection.SyncRoot { get { if (this._syncRoot == null) Interlocked.CompareExchange(ref this._syncRoot, new object(), (object) null); return this._syncRoot; } } – null_pointer Jun 29 '11 at 18:21
  • 2
    So it's just returning an empty `new object()`. . . – richard Jun 29 '11 at 18:22
  • Correct. However I still think it's safer to create that object yourself and lock it. Because you can make that object static, that object will never change if your collection instance changes. You just have more control over the lock. – null_pointer Jun 29 '11 at 18:25