Share via

Synchronization in Generic Collections [Brian Grunkemeyer]

  • Article

A few astute users of our generic collections observed that we no longer provide a public SyncRoot property on ICollection<T>.  I believe we're better off not providing it.  You often want to do synchronization at some higher level in your application, not just on one collection. I've recently been writing an FxCop rule that modifies two or three generic collections stored in static variables, and all of these updates need to be atomic. So I take a lock on one synchronization object explicitly allocated for this purpose.

One benefit here is I never have to worry about breaking any invariants in my code between the multiple collections. Say I have the invariant that if List A contains an object, then Dictionary B must also contain that object. If I took a lock on A then updated A & released the lock, then took a lock on B, updated B & released the lock, I'd have a window where my invariant wasn't true when viewed from other threads. That's not the right thing to do, and our synchronized wrapper design pattern encouraged this by allowing you to write naïve code that would update just one collection safely.

Worse yet, the synchronized wrapper pattern led some MS developers to write code like this to check whether a queue was empty:

if (queue.Count > 0) {

Object obj = null;

try {

obj = queue.Dequeue();

}

catch (Exception) { } // This swallows any indication we have a race

// Use obj here, dealing with null. }

}

Here there is a lock around both the Count property and the Dequeue method, but the lock is released by our synchronized wrapper in between these two method calls. The right fix (using our non-generic collections) is something more like this:

     Object obj = null;    lock (queue.SyncRoot) {         // In this app, locking on the queue was sufficient        if (queue.Count > 0)            obj = queue.Dequeue();    }    // use obj here, dealing with null.

To be fair, our design patterns forced this on people. If we train people to just pretend a synchronized wrapper will solve their threading problems, we're setting up our users to fall into subtle traps like this.

How does a flawed design for synchronized wrappers imply we should remove ICollection's SyncRoot property? Remember that the SyncRoot property only existed to implement these synchronized wrappers. People can still use lock(myCollection) or lock(myInternalSyncObject).

I think we're better off forcing people to think more heavily about synchronization, and frankly a SyncRoot property on our generic collections doesn't provide any value.

Comments