ICollection, readonly collections, and synchronisation. Is this right?

Question

I have a custom class that implements ICollection, and this class is readonly, ie. IsReadOnly returns true (as opposed to using the readonly keyword), and all functions that would normally modify the data in the collection throw InvalidOperationException's.

Now, given such a construct, and a quick skim over the thread-safety issues when implementing ICollection (specifically 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 won't cause different threads to lock properly, because they are getting different objects from SyncRoot. Given that this is a readonly collection though, is this an issue? Are there memory/GC issues with returning new Object()? Any other issues you can see with this implementation?

Answer 1

Yes this is an issue in some cases. Even though the collection is read only and cannot be changed, the objects the collection references are not read only. Thus if the clients use the SyncRoot to perform locking they will not be thread safe when modifying the objects referenced by the collection.

I would recommend adding:

private readonly object syncRoot = new object();

to your class. Return this as the SyncRoot and you're good to go.

Answer 2

I guess the issue would be if clients used your sync root to achieve locking of not only your collection, but something else. Supposed they cached the size of the collection - or maybe "what subset of this collection matches a predicate" - they would reasonably assume that they could use your SyncRoot to guard both your collection and their other member.

Personally I hardly use SyncRoot at all, but I think it would be sensible to always return the same thing.