C# System.Collections.Generic.Dictionary`2.Insert - An item with the same key has already been added

Question

This question is in regards to a .Net Framework 4.5 MVC Web Application.

I've got a block of code that we've inherited and been using for years that generically converts a DataTable to a List<T>, and one of the private methods gets a list of the properties for a generic class, for example:

ORIGINAL CODE

    private static Dictionary<Type, IList<PropertyInfo>> typeDictionary = new Dictionary<Type, IList<PropertyInfo>>();

    public static IList<PropertyInfo> GetPropertiesForType<T>()
    {
        //variables
        var type = typeof(T);

        //get types
        if (!typeDictionary.ContainsKey(typeof(T)))
        {
            typeDictionary.Add(type, type.GetProperties().ToList());
        }

        //return
        return typeDictionary[type];
    }

Nothing incredibly exciting going on there, it's just making sure the typeDictionary doesn't already contain the key (type) and adds it to the dictionary (key=type, value=properties), so we can access them later.

We use this code generically for any kind of "model" object, but for this particular example, this is the one that's given me trouble on 2 different occasions.

MODEL OBJECT

public class GetApprovalsByUserId
{
    // constructor
    public GetApprovalsByUserId()
    {
        TicketId = 0;
        ModuleName = string.Empty;
        ModuleIcon = string.Empty;
        ApprovalType = string.Empty;
        VIN = string.Empty;
        StockNumber = string.Empty;
        Year = 0;
        Make = string.Empty;
        Model = string.Empty;
        Services = string.Empty;
        RequestedDate = DateTime.MinValue;
    }

    // public properties
    public int TicketId { get; set; }
    public string ModuleName { get; set; }
    public string ModuleIcon { get; set; }
    public string ApprovalType { get; set; }
    public string VIN { get; set; }
    public string StockNumber { get; set; }
    public int Year { get; set; }
    public string Make { get; set; }
    public string Model { get; set; }
    public string Services { get; set; }
    public DateTime RequestedDate { get; set; }
}

Again, nothing really significant going on in that particular model class, and nothing any different than we use in any other class.

Like I said, we use this code generically in several projects, and have never once had issues with it, but on 2 separate occasions in the past day we've had it throw the following exception:

An item with the same key has already been added.

at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
at Utilities.Extensions.GetPropertiesForType[T]()
at Utilities.Extensions.ToObject[T](DataRow row)
at Utilities.Extensions.ToList[T](DataTable table)

In case it is helpful, you can see the full Extensions.cs class (static) that the extension methods live in here:

https://pavey.azurewebsites.net/resources/Extensions.txt

My questions are:

1. Given the fact that the code is already doing a !typeDictionary.ContainsKey(typeof(T)) check, how is it possible that it could ever pass that test, yet fail on the typeDictionary.Add(type, type.GetProperties().ToList()); call?

2. Why would it be so sporadic? It seemingly works 99% of the time, using the same code, the same class (GetApprovalsByUserId, shown above), and never has failed otherwise in any other project or any other model class.

We have not been able to reproduce this issue using the exact same code, model, data, or otherwise the exact same setup in any environment, so not sure how to safe-guard this code anymore than it already is.

One thought I have is to change the code to this:

PROPOSED CODE CHANGE

    private static Dictionary<Type, IList<PropertyInfo>> typeDictionary = new Dictionary<Type, IList<PropertyInfo>>();

    public static IList<PropertyInfo> GetPropertiesForType<T>()
    {
        //variables
        var type = typeof(T);
        IList<PropertyInfo> properties = null;

        //get types
        try
        {
            if (!typeDictionary.ContainsKey(type))
            {
                typeDictionary.Add(type, type.GetProperties().ToList());
            }
        }
        catch
        {
        }

        // try get value
        typeDictionary.TryGetValue(type, out properties);

        // return
        return properties;
    }

But since I can't reproduce the error in the first place, I'm not entire sure if this is bullet proof either. My thinking would be that it's just something weird with ContainsKey, particularly with using a typeof(T) as the "key", which allows it to pass the test in odd cases, when it really shouldn't, but the Add fails because it knows the key is already there. So if I try/catch it, if the ContainsKey incorrectly tells me it's not there, when it in fact is, the Add will still fail, but I'll catch it, and move on, and then I can TryParse to get the value out, and all should be well.

Appreciate any thoughts, ideas, or specifically how to reproduce the problem with the Original Code shown above, and recommended improvements to safe guard it.

Solution 1

The problem you have is concurrent access. Between the check and the insert into the dictionary, another thread has come in and added the type, causing the second insert to fail.

To fix this, you have two options: either use locks (as mentioned in the other answers) or use a ConcurrentCollection:

using System.Collections.Concurrent;
private static ConcurrentDictionary<Type, IList<PropertyInfo>> typeDictionary = new ConcurrentDictionary<Type, IList<PropertyInfo>>();

public static IList<PropertyInfo> GetPropertiesForType<T>()
{
    //variables
    var type = typeof(T);

    typeDictionary.TryAdd(type, type.GetProperties().ToList());

    //return
    return typeDictionary[type];
}

This will add the value if it doesn't yet exist and return true, otherwise it will not do anything and return false

Solution 2

You need an object to lock on:

private object lockObj = new object();

And then you need to lock before adding a key:

if (!typeDictionary.ContainsKey(typeof(T)))
{
    lock(lockObj) 
    {
        if (!typeDictionary.ContainsKey(typeof(T)))
        {
            typeDictionary.Add(type, type.GetProperties().ToList());
        }
    }
}

This will make any other threads that are looking for the same key wait if it's already in the process of being added. You check ContainsKey again inside the lock because when a stopped thread finally acquires the lock another thread might have already inserted the key.

This is an example of double check locking