4

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.

Robert Harvey
  • 178,213
  • 47
  • 333
  • 501
Matt
  • 43
  • 1
  • 1
  • 6
  • 5
    `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?` -- **Race condition?** – Robert Harvey Oct 12 '16 at 16:53
  • 3
    `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.` -- **Race condition?** – Robert Harvey Oct 12 '16 at 16:54
  • Definitely race condition. You need to `lock` around the code that checks if the key exists already and inserts it – Matt Burland Oct 12 '16 at 16:57
  • Robert and Matt, incredibly helpful insight on this. I guess there is no good way to simulate/test this before/after the code is changed to prove it beyond a reasonable doubt? – Matt Oct 12 '16 at 17:12
  • Oh, it's very easy to simulate race conditions, just not in your application. Most of the code below is orthodox enough to use confidently, but if you really need proof, put it in a small console application and throw a couple of threads at it. – Robert Harvey Oct 12 '16 at 17:15
  • @Matt: Yeah, these can be a pain to test. As Robert suggested, your best bet is to throw together a test bed to just throw a bunch of threads at that piece of code. I actually ran in to a [very similar problem myself](http://stackoverflow.com/questions/23408921/null-reference-in-web-api-call) some time ago. It wasn't a key existing problem (because I didn't actually use `Add`) but a dictionary resizing problem. – Matt Burland Oct 12 '16 at 17:37
  • Matt - thanks for the follow, and the reference to your issue. This is all really great information. I've dealt with some threading issues before, but this one really had me confused, and all the feedback was spot on between you, Robert, and Kenneth, and I'm already using the ConcurrentDictionary approach. Definitely became so obvious once you all jumped in. Was disappointed to see the other non-accepted answer/comments disappear, it had a lot of good info on locks vs. double-locks vs. ConcurrentDictionary and pros/cons to each. Thanks all! – Matt Oct 12 '16 at 17:50

2 Answers2

4

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

Kenneth
  • 28,294
  • 6
  • 61
  • 84
  • 3
    https://arbel.net/2013/02/03/best-practices-for-using-concurrentdictionary/ great article on the advantages of ConcurrentDictionary over lock dictionaries. – David L Oct 12 '16 at 17:18
1

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

Matt Burland
  • 44,552
  • 18
  • 99
  • 171