CODEWITHSUNDEEP
c#.netmultithreading.net-4.0dictionary
Avalanchis
Avalanchis

Reputation: 4579

Exception when adding Dictionary entry

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?

Upvotes: 8

Views: 2879

Answers (3)

Fredrik M&#246;rk
Fredrik M&#246;rk

Reputation: 158389

If you are on .NET Framework 4 or later, I would suggest that you use a ConcurrentDictionary instead. The TryAdd method keeps you safe from this kind of scenario, without the need to litter your code with locks:

localSerializers.TryAdd(sessionObjectName, CreateSerializer(serializerType))

If you are worried about CreateSerializer to be invoked when it's not needed, you should instead use AddOrUpdate:

localSerializers.AddOrUpdate(
    sessionObjectName,
    key => CreateSerialzer(serializerType),
    (key, value) => value);

This will ensure that the method is called only when you need to produce a new value (when it needs to be added to the dictionary). If it's already present, the entry will be "updated" with the already existing value.

Upvotes: 1

to StackOverflow
to StackOverflow

Reputation: 124804

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.

Upvotes: 5

sll
sll

Reputation: 62564

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

Upvotes: 5

Related Questions