arrow-left arrow-right brightness-2 chevron-left chevron-right circle-half-full dots-horizontal facebook-box facebook loader magnify menu-down RSS star Twitter twitter GitHub white-balance-sunny window-close
C#: Locking when calling external code
2 min read

C#: Locking when calling external code

This is an old post and doesn't necessarily reflect my current thinking on a topic, and some links or images may not work. The text is preserved here for posterity.

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? :)

Paul Stovell's Blog

Hello, I'm Paul Stovell

I'm a Brisbane-based software developer, and founder of Octopus Deploy, a DevOps automation software company. This is my personal blog where I write about my journey with Octopus and software development.

I write new blog posts about once a month. Subscribe and I'll send you an email when I publish something new.

Subscribe

Comments