ICollection, read-only collections and synchronization. It is right?

Question

I have a custom class that implements ICollection , and this class is readonly, i.e. IsReadOnly returns true (as opposed to using the readonly keyword) and all functions that typically modify data in the throw collection of InvalidOperationException .

Now, given this design and quickly bypassing thread safety issues when implementing ICollection (in particular ICollection.IsSynchronized and friends), I came up with this quick and dirty solution.

 bool ICollection.IsSynchronised { get{ return true; } } object ICollection.SyncRoot { get{ return new Object(); } } 

Now, given the examples in MSDN, this will not cause different threads to block correctly, because they get different objects from SyncRoot . Given that this is a read-only assembly, is this a problem? Are there any memory / GC problems with returning new Object() ? Any other problems you can see with this implementation?

Answer 1

Yes, this is a problem in some cases. Although the collection is read-only and cannot be modified, objects whose collection links are not readable. Thus, if clients use SyncRoot to perform the lock, they will not be thread safe when modifying the objects referenced by the collection.

I would recommend adding:

 private readonly object syncRoot = new object(); 

for your class. Bring it back as SyncRoot and you will go well.

Answer 2

It seems very strange to return different objects every time ... in fact, I very rarely (if ever) used the SyncRoot approach, since often I need to synchronize between multiple objects, etc., so a separate object is more significant.

But if the data is really immutable (read-only), why not just return false from IsSynchronized?

Re GC - any such object is usually short-lived and assembled in GEN0. If you have a field with an object (for blocking), it will continue until the collection, but most likely it does not hurt ...

If you need a lock, I’ll be tempted to just:

 private readonly object lockObj = new object(); 

You can also use the lazy approach only to the “new” when necessary, which makes some sense if you really don't expect anyone to ask for synchronization (by returning false to IsSynchronized).

Another common approach is to return "this"; it keeps things simple, but the risks conflict with some other code, using your object as a lock for an unrelated purpose. Rare, but possible. This is actually the approach that [MethodImpl] uses for synchronization.

Answer 3

I suppose the problem will be that the clients used your synchronization root to achieve blocking not only your collection, but also something else. Suppose they cached the size of the collection — or perhaps “which subset of this collection matches the predicate” —they reasonably assume that they can use your SyncRoot to protect your collection and its other member.

Personally, I hardly use SyncRoot, but I think it would be wise to always return the same.