Exception when adding Dictionary entry

Question

We're seeing this exception occur in the following block of code in an ASP.NET context which is running on an IIS 7 server.

1) Exception Information
*********************************************  
Exception Type: System.Exception  
Message: Exception Caught in Application_Error event
Error in: InitializationStatus.aspx  
Error Message:An item with the same key has already been added.  
Stack Trace:    at
System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)   
at CredentialsSession.GetXmlSerializer(Type serializerType)

This is the code that the exception is occuring in:

[Serializable()]
public class CredentialsSession
{
    private static Dictionary<string, System.Xml.Serialization.XmlSerializer> localSerializers = new Dictionary<string, XmlSerializer>();

    private System.Xml.Serialization.XmlSerializer GetXmlSerializer(Type serializerType)
    {
        string sessionObjectName = serializerType.ToString() + ".Serializer";

        if (Monitor.TryEnter(this))
        {
            try
            {
                if (!localSerializers.ContainsKey(sessionObjectName))
                {
                    localSerializers.Add(sessionObjectName, CreateSerializer(serializerType));
                }
            }
            finally
            {
                Monitor.Exit(this);
            }
        }
        return localSerializers[sessionObjectName];
    }

    private System.Xml.Serialization.XmlSerializer CreateSerializer(Type serializerType)
    {
        XmlAttributes xmlAttributes = GetXmlOverrides();

        XmlAttributeOverrides xmlOverrides = new XmlAttributeOverrides();
        xmlOverrides.Add(typeof(ElementBase), "Elements", xmlAttributes);

        System.Xml.Serialization.XmlSerializer serializer =
            new System.Xml.Serialization.XmlSerializer(serializerType, xmlOverrides);

        return serializer;
    }
}

The Monitor.TryEnter should be preventing multiple threads from entering the block simultaneously, and the code is checking the Dictionary to verify that it does not contain the key that is being added.

Any ideas on how this could happen?

Answer 1

Your code is not thread-safe.

1. You're locking on this, a CredentialsSession instance, but accessing a static dictionary which can be shared by multiple CredentialsSession instances. This explains why you're getting the error - two different CredentialsSession instances are attempting to write to the dictionary concurrently.

2. Even if you change this to lock on a static field as suggested in @sll's answer, you aren't thread-safe, because you aren't locking when reading the dictionary. You need a ReaderWriterLock or ReaderWriterLockSlim to efficiently allow multiple readers and a single writer.

   Therefore you should probably use a thread-safe dictionary. ConcurrentDictionary as others have said if you're using .NET 4.0. If not you should implement your own, or use an existing implementation such as http://devplanet.com/blogs/brianr/archive/2008/09/26/thread-safe-dictionary-in-net.aspx.

Your comments suggest you want to avoid calling CreateSerializer for the same type multiple times. I don't know why, because the performance benefit is likely to be negligible, since contention is likely to be rare and can't exceed once for each type during the lifetime of the application.

But if you really want this, you can do it as follows:

var value;
if (!dictionary.TryGetValue(key, out value))
{
    lock(dictionary)
    {
        if(!dictionary.TryGetValue(key, out value))
        {
            value = CreateSerializer(...);
            dictionary[key] = value;
        }
    }
}

From comment:

if I implement this with ConcurrentDictionary and simply call TryAdd(sessionObjectName, CreateSerializer(serializerType)) every time.

The answer is not to call TryAdd every time - first check if it's in the dictionary, then add if it isn't. A better alternative might be to use the GetOrAdd overload that takes a Func argument.

Answer 2

Try out locking on localSerializers rather that this. BTW, why you are using Monitor explicitly? Only one reason I see is to provide lock timeout which obviously you are not using, so use simply lock() statement instead this would generate try/finally as well:

lock (localSerializers)
{
   if (!localSerializers.ContainsKey(sessionObjectName))                 
   {                     
      localSerializers.Add(
            sessionObjectName, 
            CreateSerializer(serializerType));                 
   } 
}

EDIT: Since you've not specified in tags that you're using .NET 4 I would suggest using ConcurrentDictionary<TKey, TValue>

---

Monitor.Enter() Method:

Use a C# try…finally block (Try…Finally in Visual Basic) to ensure that you release the monitor, or use the C# lock statement (SyncLock statement in Visual Basic), which wraps the Enter and Exit methods in a try…finally block