how to make this c# class a singleton, thread safe

Question

So here's what i'm trying to do :

public static class MyClass
{

   private Dictionary<string,IDisposable> dict = null;

   static MyClass()
   {
         dict = new Dictionary<string,IDisposable>();

         // i fill up the dictionary in a loop here
         //
   }

   public static UseDictionary(string name)
   {
       var obj = dict[name]        
       obj.DoSomeAction();           
   }

}

static void Main(string[] args)
{
   Parallel.For(0, 1000, x =>
   {               
       // assuming 'name' does exist in the dictionary 
       MyClass.UseDictionary("name"); // randomly throws null reference errors 
   });
}

I basically want to have a single instance of this class which will initialize the dictionary only once (the IDisposable item is an expensive remote connection i'm opening and i want to open it only once )

This class will be used by different asp.net pages. So i want it to be thread safe and singleton. I'm thinking initializing the dictionary in the static constructor will make it only be called once and is thread safe, not sure why i keep getting errors. Any ideas?

Perhaps sharing the exact text of the error messages could be beneficial to those trying to help you. — Steve Wellens, Mar 30 '12 at 02:14
From the example the only thing that may be causing that is some issue with DoSomething method that is being called concurrently by many threads. — Leonardo Garcia Crespo, Mar 30 '12 at 02:21
possible duplicate of [Is this a valid, lazy, thread-safe Singleton implementation for C#?](http://stackoverflow.com/questions/2615527/is-this-a-valid-lazy-thread-safe-singleton-implementation-for-c) — Joe, Mar 30 '12 at 02:26
Please take a look here for good explanations of singleton patterns : http://csharpindepth.com/Articles/General/Singleton.aspx — Stefan H, Mar 30 '12 at 02:37
Usually singleton has a property "Get" or "Instance" that returns itself. So you can call it using MyClass.Instance.UseDictionary("name") — Daniel Imms, Mar 30 '12 at 02:38
Note, your private variable should be static, as you can't access instance fields from static methods. — Dominic Zukiewicz, Mar 30 '12 at 12:20
And just another point. Applying locks negates the use the Parallel.ForEach() — Dominic Zukiewicz, Mar 30 '12 at 12:22
@BobHorn, my issue was with the obj i was using being not thread safe. — newbie, Mar 31 '12 at 23:38

tvanfosson · Accepted Answer · 2012-03-30T03:04:47.403

I agree with @Leonardo Garcia Crespo's comment. From what I see here, you only write to the dictionary in the static constructor, which will only be called once per app domain. Everything else is a read and the generic dictionary supports multiple readers if the collection isn't modified. The problem could only lie in the code that's being invoked. It also needs to be thread-safe. Alternatively, you could use another dictionary of associated locks and lock the corresponding lock so that you can only call one non-thread-safe instance at a time.

public static class MyClass
{
    public static Dictionary<string,SomeDisposableClass> dict = null;
    public static Dictionary<string,object> locks = null;

    static MyClass()
    {
         // populate the dictionary 
         locks = new Dictionary<string,object>();
         foreach (var key in dict.Keys)
         {
             locks[key] = new object();
         }
    }

    public static void UseDictionary( string name )
    {
        var obj = dict[name];
        var sync = locks[name];
        lock(sync)
        {
            obj.DoSomething();
        }
    }
}

You could also use a Singleton pattern, but if you only have the one static method that's probably not necessary. I will note that this will be horrible to work around for any code that uses this class.

And, yes, I know that it will be single threaded if called 1000 times with the same value for name. If the method that's being called isn't thread-safe, though, that's the best you can do. This way, at least, you can have multiple threads operating for different values of name at the same time since they lock on different objects.

i do have 10-20 more static method, but i just showed one example here. I think the problem might be in the DoSomething method like you said. I'm wondering if this is the right pattern though.What i'm trying to accomplish is to use this class as the one and only place where connections are opened. Any consumer of this class does not need to worry about opening connections to different servers. Is there some other pattern you would use to design this class? — newbie, Mar 30 '12 at 03:51
Is Sigleton pattern better or using a static constructor like i'm doing? Especially for asp.net web pages? — newbie, Mar 30 '12 at 03:56
@newbie my preference would be for it not to be a singleton or static class at all. Yes, put the logic in one place, but have the class be a normal class. If the operations really are that expensive, and I'd need to be shown that performance was really a problem, then you might consider caching or dependency injection, where a single instance can be injected. I'd be concerned though that your connections wouldn't survive the lifetime of the object and would probably opt to let them be recreated for each page. — tvanfosson, Mar 30 '12 at 10:53
the issue was that my dictionary items 'obj' were not thread safe. using locks around the usage addresses the problem like @tvanfosson suggests — newbie, Mar 31 '12 at 23:33

score 0 · Answer 2 · edited May 23 '17 at 12:27

Look at here http://www.codeproject.com/Articles/14026/Generic-Singleton-Pattern-using-Reflection-in-C

Sorry, :'( The road to hell is filled with good intentions

i just want give other one simple option without entering into much detail

Time after i also had is problem and i post that Multi web services so multi singleton finally create my own version.

That really work i have around to 50 internal applications and some external providers what used it.

Well in summary After the check ideas about the singleton pattern and diferents examples done in c# i arrived to the conclusion that a good implementation is based in have internal class for create one instanced is guarantees that called to the constructor is done once time although the singleton clas is called from many threads so just have one instance

In vb.net simplified

Public NotInheritable Class ServiceProxySingleton

 Private Sub New()
 End Sub

 Public Shared Function GetInstance() As ServiceProxySingleton
   Return NestedSingletonService._instance
 End Function

  ' Internal class
  Class NestedSingletonService
    Friend Shared ReadOnly _instance As [SingletonClass] = New [SingletonClass]()
    Shared Sub New()
    End Sub
  End Class

End Class

Which is a very heavy version that requires downloading someone elses code of what I suggested. It's generally best to make your own code and understand it rather then download something and plug it in no? (in the context of small fixes like this) — , Mar 30 '12 at 02:23
Please do not post links as answers, also explain the content of your link so that if that link ever goes down, there is still a record of what your answer is. — Stefan H, Mar 30 '12 at 02:24
well well the link is provided for pure information understand or copy it depends of each one — Carlos Cocom, Mar 30 '12 at 20:16

score 0 · Answer 3 · answered Mar 30 '12 at 11:42

The following code should be threadsafe, becuase .NET guarantees thread safety for static field initializers

private static readonly Dictionary<string,IDisposable> dict = 
                                           CreateAndInitializeDictionary();
private static Dictionary<string,IDisposable> CreateAndInitializeDictionary() {
   var d = new Dictionary<string,IDisposable>();
   .... // here add items 
   return d;
}

And after reviewing your code one more time I realized that your implementation is also thread safe. The issue should be somewhere in DoSomething() :)

score 0 · Answer 4 · answered Mar 30 '12 at 12:17

You could use the Synchronization attribute on your class. This is what I did for one of my classes. I included the comments because they explain the situation.

[Synchronization]
public class IprServerController : ContextBoundObject
{
    // We need to worry about things like a user deleting a route at the same time that another user is starting one.
    // So, instead of trying to lock appropriately throughout the class, we can lock declaratively, by using the
    // Synchronization attribute and deriving from ContextBoundObject. Now only one thread can execute methods on
    // this class at a time.

how to make this c# class a singleton, thread safe

4 Answers4

Linked