Why using ReaderWriterLockSlim doesn't make my Dictionary thread safe?

Question

I wrote a small piece of code that rapidly read and write to a dictionary from multiple threads. I used ReaderWriterLockSlim to protect the code and still received an exception for allegedly trying to add duplicate keys.

ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
Dictionary<int, int> _dict = new Dictionary<int, int>();

public SafeDictionaryTester()
{
    for (int i = 0; i < 7; i++)
    {
        _dict.Add(i, i);
    }
}

internal void Execute()
{
    for (int i = 7; i < 10000; i++)
    {
        if (i % 6 == 0)
            new Thread(new ThreadStart(delegate { Print(6); })).Start();
        else if (i % 5 == 0)
            new Thread(new ThreadStart(delegate { Print(5); })).Start();
        else if (i % 4 == 0)
            new Thread(new ThreadStart(delegate { Print(4); })).Start();
        else if (i % 3 == 0)
            new Thread(new ThreadStart(delegate { Print(3); })).Start();
        else if (i % 2 == 0)
            new Thread(new ThreadStart(delegate { Print(2); })).Start();
        else if (i % 1 == 0)
            new Thread(new ThreadStart(delegate { Print(1); })).Start();

        new Thread(new ThreadStart(delegate
        {
            _lock.EnterWriteLock();
            try
            {
                _dict.Add(i, i); // Exception after random number of loops
                Console.WriteLine(i.ToString() + " added");
            }
            finally
            {
                _lock.ExitWriteLock();
            }
        })).Start();
    }
}

private void Print(int i)
{
    _lock.EnterReadLock();
    try
    {
        int obj;
        if (_dict.TryGetValue(i, out obj))
        {
            Console.WriteLine(obj);
        }
        else
        {
            throw new Exception();
        }
    }
    finally
    {
        _lock.ExitReadLock();
    }
}

Note that the exact code without the threads executes perfectly.

Answer 1

As Ani says, it's got little to do with the dictionary. You really are (probably) trying to add the same key twice, because you're capturing the loop variable. The simple fix is to copy the loop variable to a new variable inside the loop so that each extra thread will only "see" its own value.

for (int i = 7; i < 10000; i++)
{
    // Other stuff...
    copyOfI = i;

    new Thread(new ThreadStart(delegate
    {
        _lock.EnterWriteLock();
        try
        {
            _dict.Add(copyOfI, copyOfI);
            Console.WriteLine(copyOfI.ToString() + " added");
        }
        finally
        {
            _lock.ExitWriteLock();
        }
    })).Start();
}

See Eric Lippert's blog posts for more information: part 1; part 2.

Answer 2

The problem is that your anonymous writer delegate creates a closure over i.

That is to say, when your writer threads execute, they'll use the current value of i rather than the value at the time the thread was started (7, 8, 9 ... etc.)

To fix it, you need to make a copy of the variable inside your for loop and use that in your writer delegate:

internal void Execute()
{
    for (int i = 7; i < 10000; i++)
    {
        // trimmed for brevity: create a copy of i
        int copy = i;

        new Thread(new ThreadStart(delegate
        {
            _lock.EnterWriteLock();
            try
            {
                _dict.Add(copy, copy); // Exception after random number of loops
                Console.WriteLine(copy.ToString() + " added");
            }
            finally
            {
                _lock.ExitWriteLock();
            }
        })).Start();
    }