CODEWITHSUNDEEP
c#multithreading
Claus Jørgensen
Claus Jørgensen

Reputation: 26338

Exposing GetEnumerator() for a concurrent dictionary

I'm working on a concurrent dictionary implementation for C#, and I'm wondering if this GetEnumerator() implementation is actually (thread) safe.

It's not doing a actual snapshot, so I'm wondering if it'll mess up later read/writes to the internal dictionary, or if it'll expose potential deadlocks because the enumeration of the exposed IEnumerator will actually run inside the lock. 

private readonly Dictionary<TKey, TValue> internalDictionary;
private SpinLock spinLock = new SpinLock();

IEnumerator IEnumerable.GetEnumerator()
{
    IEnumerator enumerator;

    bool lockTaken = false;
    try
    {
        spinLock.TryEnter(ref lockTaken);
        enumerator = (this.internalDictionary as IEnumerable).GetEnumerator();
    }
    finally
    {
        if (lockTaken)
        {
            spinLock.Exit(false);
        }
    }

    return enumerator;
}

Upvotes: 2

Views: 2167

Answers (2)

usr
usr

Reputation: 171188

Your method is not thread-safe with respect to concurrent writers because

  1. Your enumerator is not snapshotting anything. It is referring back the original dictionary. Call ToList on it or something to actually snapshot.
  2. Concurrent writers do not use your lock so they execute concurrently. This is unsafe.
  3. Don't use a spinlock if the lock body is big.
  4. What if TryEnter fails? It is called try after all.

Here is a fixed version with all the cleverness removed:

IEnumerator IEnumerable.GetEnumerator()
{
    lock (internalDictionary) return internalDictionary.ToList();
}

Concurrent writers have to take the lock, too.

Upvotes: 3

Morten Mertner
Morten Mertner

Reputation: 9474

The first things that springs to mind is why on earth you would want to create such a beast yourself when .NET ships with thread-safe concurrent collection classes (including a dictionary). Google System.Collections.Concurrent for more information.

I've previously benchmarked the ConcurrentDictionary class and I'll guarantee you that they are significantly faster than anything you could write yourself, even when using spin locks or Interlocked to avoid any of the heavier locking mechanisms.

In answer to your question, maybe, but it depends on the implementation (which is never a good thing). I'd guess that any write attempts would fail, since the standard collection classes cannot be modified while being iterated; but I wouldn't rely on any such mechanism to keep my own code safe.

Upvotes: 1

Related Questions