C#: Locking when calling external code

Can you spot the danger in the following code?

public void HandleAdd(IEnumerable<T> addedItems) 
{
    lock (_itemsLock) 
    {
        foreach (T item in addedItems) 
        {
            _items.Add(item);
        }
    }
}

Don't see it? Consider the following:

1. Thread A, a background thread, is synchronizing changes made in-memory with the database:
   • It already holds a lock on a class called "ChangeQueue".
   • It calls HandleAdd(), passing in a list of changes.
2. Thread B, the UI thread, is searching for something:
   • It calls GetItem(), which acquires the lock for _itemLock.
   • The delegate it passed to GetItem() accesses the ChangeQueue

See the problem now?

This is a deadlock scenario - thread A holds one lock and wants another, while thread B holds the second lock but wants the first. Neither can continue, the window stops processing messages, and Windows kindly renames your application to "(Not Responding)" and gives it a pretty shade of white :)

To lock A, or to lock B: that is the question

The common advice for these scenarios is that you should always acquire locks in the same order. Indeed, that's easy to do within the same class. In this scenario above, however, I only wrote code for one of the locks. As the author of that code, I don't even know about the other class or that it takes locks. What can I do to avoid deadlocks?

I learnt this the hard way with Bindable LINQ. The code-base was littered with code like the above, until a couple of the unit tests I wrote to test threading started to fail just once every now and then. It took a while, but I eventually tracked it down to this pattern, and created my rule:

Never invoke code you don't control whilst holding a lock.

The danger is that any code supplied externally could try to gain a lock themselves, and if you already hold one, you run the risk of deadlock. Delegates passed to methods, objects implementing an interface, or even calling virtual methods on classes you wrote yourself, can spell danger. For example, the IEnumerable<T> passed into the HandleAdd() method could try to acquire a lock in the GetEnumerator() method.

Avoidance

To avoid this problem, there are three tricks I normally use:

1. Avoid locking where possible
2. Take snapshots of the arguments, or invoke the methods before locking
3. Build snapshots of my internals while holding a lock, then invoke outside methods after

For example, the method above could be re-written as follows:

public void HandleAdd(IEnumerable<T> addedItems) 
{
    List<T> items = new List<T>();
    items.AddRange(addedItems);

    lock (_itemsLock) 
    {
        foreach (T item in items) 
        {
            _items.Add(item);
        }
    }
}

While not as efficient as the first, by executing the external code outside of the locks, I've avoided any accidental deadlock caused by interaction between my code and external code. You might consider caching the snapshots for next time - then you only pay when they have changed, versus every time you iterate.

Out of interest, I couldn't find an FxCop rule for this. Anyone interested in writing one? :)