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

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.

Answer 2

It seems very odd to return a different object each time... actually, I very rarely (if ever) use the SyncRoot approach, as often I need to synchronize between multiple objects etc, so a separate object is more meaningful.

But if the data is truly immutable (readonly), why not just return false from IsSynchronized?

Re GC - any such object would typically be short lived and be collected in GEN0. If you have a field with an object (for the lock), it would last as long as the collection, but most likely won't hurt anyway...

If you need a lock, I'd be tempted to just have a:

private readonly object lockObj = new object();

You could also use a lazy approach to only "new" it when needed, which makes a certain amount of sense if you don't actually expect anyone to ask for the sync-lock (by returns false to IsSynchronized).

Another common approach is to return "this"; it keeps things simple, but risks conflicts 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 to synchronize.

Answer 3

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.