Throw a NullReferenceException while calling the set_item method of a Dictionary object in a multi-threading scenario

Our website has a configuration page such as "config.aspx", when the page initializing will load some information from a configuration file. To cache the loaded information we provided a factory class and we call a public method of the factory to get the configuration instance when the page loaded. But sometimes when the Application Pool is restarted, we found some error message in the Event Log such as below:

Message: Object reference not set to an instance of an object.
Stack:   at System.Collections.Generic.DictionaryMessage: Object reference not set to an instance of an object.
Stack:   at System.Collections.Generic.Dictionary2.Insert(TKey key, TValue value, Boolean add)    at System.Collections.Generic.Dictionary2.set_Item(TKey key, TValue value)
at ObjectFactory.GetInstance(string key)
at config.Page_Load(Object sender, EventArgs e)
at System.Web.Util.CalliHelper.EventArgFunctionCaller(IntPtr fp, Object o, Object t, EventArgs e)
at System.Web.Util.CalliEventHandlerDelegateProxy.Callback(Object sender, EventArgs e)
at System.Web.UI.Control.OnLoad(EventArgs e)
at System.Web.UI.Control.LoadRecursive()
at System.Web.UI.Page.ProcessRequestMain(Boolean includeStagesBeforeAsyncPoint, Boolean includeStagesAfterAsyncPoint)
2.set_Item(TKey key, TValue value)
at ObjectFactory.GetInstance(string key)
at config.Page_Load(Object sender, EventArgs e)
at System.Web.Util.CalliHelper.EventArgFunctionCaller(IntPtr fp, Object o, Object t, EventArgs e)
at System.Web.Util.CalliEventHandlerDelegateProxy.Callback(Object sender, EventArgs e)
at System.Web.UI.Control.OnLoad(EventArgs e)
at System.Web.UI.Control.LoadRecursive()
at System.Web.UI.Page.ProcessRequestMain(Boolean includeStagesBeforeAsyncPoint, Boolean includeStagesAfterAsyncPoint)

The factory class implements like below:

public static class ObjectFactory
{
private static object _InternalSyncObject;
private static Dictionary _Instances;



private static object InternalSyncObject
{
    get
    {
        if (_InternalSyncObject == null)
        {
            var @object = new object();
            Interlocked.CompareExchange(ref _InternalSyncObject, @object, null);
        }

        return _InternalSyncObject;
    }
}

private static Dictionary<string, object> Instances
{
    get
    {
        if (_Instances == null)
        {
            lock (InternalSyncObject)
            {
                if (_Instances == null)
                {
                    _Instances = new Dictionary<string, object>();
                }
            }
        }

        return _Instances;
    }
}

private static object LoadInstance(string key)
{
    object obj = null;

    // some statements to load an specific instance from a configuration file.

    return obj;
}

public static object GetInstance(string key)
{
    object instance;

    if (false == Instances.TryGetValue(key, out instance))
    {
        instance = LoadInstance(key);

        Instances[key] = instance;
    }

    return instance;
}




}

}

I guess the exception was thrown by the line "Instances[key] = instance;", because its the only code that could call set_Item method of a dictionary. But if the "Instances" value is null, it will thrown a NullReferenceException when calling the TryGetValue method and the top frame of the stacktrace should be the GetInstance not the Insert. Does anyone know how the dictionary could throw a NullReferenceException when call the set_Item method in multi-threading scenario?

As the exception occurs internally in the Dictionary code, it means that you are accessing the same Dictionary instance from more than one thread at the same time.

You need to synchronise the code in the GetInstance method so that only one thread at a time accesses the Dictionary.

Edit:
Lock around the accesses separately, so that you aren't inside a lock while doing the (supposedly) time consuming loading:

private static object _sync = new object();

public static object GetInstance(string key) {
   object instance = null;
   bool found;
   lock (_sync) {
      found = Instances.TryGetValue(key, out instance);
   }
   if (!found) {
      instance = LoadInstance(key);
      lock (_sync) {
         object current;
         if (Instances.TryGetValue(key, out current)) {
            // some other thread already loaded the object, so we use that instead
            instance = current;
         } else {
            Instances[key] = instance;
         }
      }
   }
   return instance;
}

As of .Net 4 you have [ConcurrentDictionary] 1 which is a thread-safe dictionary, no more need for "manual" synchronization.

To quote <http://msdn.microsoft.com/en-us/library/xfhwa508.aspx> (emphasis added by me):

"Thread Safety

Public static (Shared in Visual Basic) members of this type are thread safe. Any instance members are not guaranteed to be thread safe.

A Dictionary<(Of <(TKey, TValue>)>) can support multiple readers concurrently, as long as the collection is not modified. Even so, enumerating through a collection is intrinsically not a thread-safe procedure. In the rare case where an enumeration contends with write accesses, the collection must be locked during the entire enumeration. To allow the collection to be accessed by multiple threads for reading and writing, you must implement your own synchronization."

I think your Instances Dictionary is not null. Your exception is from inside the Insert method - meaning that there is a Dictionary object at runtime (Also, as you've said, you have already had TryGetValue before, on the same reference) Could it be that your key is null?

EDIT

Just checked it - TryGetValue throws ArgumentNullException when it receives a null key, and so does insert with a null key. But what class are you using in your example? I used the generic IDictionary<string, string>, but I see you are using a non generic one. Is it a class you have inherited from DictionaryBase or maybe HashTable?

A better solution would be to create a synchronized dictionary. Here is one that will work in this situation. The ReaderWriterLockSlim I think is the best synchronization object to use in this situation. The writing to the dictionary will be pretty rare. Most of the time the key will be in the dictionary. I didn't implement all them methods of the dictionary, just the ones that were used in this case, so it isn't a complete synchronized dictionary.

public sealed class SynchronizedDictionary<TKey, TValue>
{
	private readonly Dictionary<TKey, TValue> dictionary = new Dictionary<TKey, TValue>();
	private readonly ReaderWriterLockSlim readerWriterLock = new ReaderWriterLockSlim();

	public TValue this[TKey key]
	{
		get
		{
			readerWriterLock.EnterReadLock();
			try
			{
				return this.dictionary[key];
			}
			finally
			{
				readerWriterLock.ExitReadLock();
			}
		}
		set
		{
			readerWriterLock.EnterWriteLock();
			try
			{
				this.dictionary[key] = value;
			}
			finally
			{
				readerWriterLock.ExitWriteLock();
			}
		}
	}

	public bool TryGetValue(TKey key, out TValue value)
	{
		readerWriterLock.EnterReadLock();
		try
		{
			return this.dictionary.TryGetValue(key, out value);
		}
		finally
		{
			readerWriterLock.ExitReadLock();
		}
	}
}