This blog has moved to Medium

Subscribe via email


Don’t switch your lock object mid-lock

Today I encountered a weird exception from List.AddRange()

[Exception(System.ArgumentException)]:
Destination array was not long enough. Check destIndex and length, and the array's lower bounds.

Looking at the code, I saw it multithreaded like this:

List<int> tmp;
lock (_list)
{
    tmp = _list;
    _list = new List<int>(1);
}
 
// use tmp object like it's owned exclusively by this thread

The thinking here was to avoid “expensive” operations inside the lock, so we locked _list only for enough time to replace it with a new list, and then do the heavy lifting on the tmp variable (assuming no other threads can interfere). Most locks are used immutably – the lock object itself rarely changes identity.

Well, it appears the above code is indeed not thread safe. Here what I recon happened:

  1. Thread 1 acquired a lock on _list
  2. Thread 2 tried acquiring the lock, but blocked.
  3. Thread 1 changed _list to a new instance and released the lock on the old value of _list
  4. Thread 3 came and acquired the lock on the new value of _list
  5. Thread 2 was now released and acquired the lock on the old value of _list

At this point, both thread 2 & 3 got inside the critical section and grabbed the same value of _list (the new one) as their “exclusive” tmp, and then worked on it concurrently outside the lock. The easy solution is not to lock on an object whose identity (not state) changes. In this case, a lock either on the containing object or on a new custom object (object _lock = new object()) should do the trick.

This program reproduces the problem:

using System;
using System.Collections.Generic;
using System.Threading;
 
namespace tstlock
{
    public class LockTester
    {
        private List<int> _list = new List<int>(1);
 
        public void Run()
        {
            var threads = new Thread[40];
            for (int i = 0; i < threads.Length; ++i)
            {
                threads[i] = new Thread(ThreadFunc);
                threads[i].Start();
            }
            foreach (var thread in threads)
                thread.Join();
        }
 
        private void ThreadFunc()
        {
            for (int i = 0; i < 1000000; ++i)
            {
                List<int> tmp;
                lock (_list)
                {
                    tmp = _list;
 
                    // at this point _list is always supposed to be an empty list
                    // because all the additions to it are after the new list is allocated
                    _list = new List<int>(1);
                }
 
                var array = new []{1,2,3,54,5,6};
                tmp.AddRange(array);
                if (tmp.Count != array.Length)
                {
                    throw new Exception("Bug - tmp is not local as we thought");
                }
            }
        }
    }
}

3 Comments

  1. Asaf E.:

    Why aren’t you sleeping?

  2. ripper234:

    The timestamp on this post is weird, I posted it around midday last night.

  3. vssdbd:

    incest 3d 3d incest art 3d incest porn 3d incest comics 3d incest xxx 3d incest drawing 3d incest xxx 3d incest comix
    [URL=http://forums.buddytv.com/members/incest_3d.html]incest 3d 3d incest art 3d incest porn 3d incest comics 3d incest xxx 3d incest drawing 3d incest xxx 3d incest comix[/URL]

    incest 3d 3d incest art 3d incest porn 3d incest comics 3d incest xxx 3d incest drawing 3d incest xxx 3d incest comix
    [URL=http://forums.buddytv.com/members/incest_3d.html]incest 3d 3d incest art 3d incest porn 3d incest comics 3d incest xxx 3d incest drawing 3d incest xxx 3d incest comix[/URL]