Thread Safe Singleton Pattern - C# ???

I am trying to implement a thread safe singleton...

Can anyone tell me if I am thinking about this properly...

I was thinking I could also use a generic method to accomplish this, but I don't know generics well enough yet...

Anyway, will this do the trick?

public sealed class Foo
{

   private static Foo instance = null;
   private static System.Threading.ReaderWriterLock instanceLock = new ReaderWriterLock();

   private Foo(){}

   public static Foo GetInstance() 
   {
      instanceLock.AcquireReaderLock (System.Threading.Timeout.Infinite);
      try
      {
         if (instance == null)
         {
            instanceLock.UpgradeToWriterLock(System.Threading.Timeout.Infinite);
            instance = new Foo();
         }
      }
      finally
      {
         instanceLock.ReleaseLock();
      }   
      return instance;
   }

}

I predict now that a lot of people are going to post really complicated examples. A thread-safe singleton is very difficult to get right when you do your own locking. For example, your code has a race condition where two threads pass the instance==null check and both create a new instance. Also the reader lock is not necessary and leads to performance loss because you're locking when you don't have to. There are ways around that (double-check locking) but they depend on memory model semantics and the slightest variation can have unforeseen circumstances.

But all those complicated examples are not in fact necessary. There's a very simple way to make a thread-safe singleton in .Net:

public sealed class Foo
{
   private static Foo _instance = new Foo();

   private Foo() { }

   public static Foo Instance
   {
      get
      {
         return _instance;
      }
   }
}

What you need to know about this:

• It's thread-safe, because the CLR guarantees that static constructors run synchronized.
• It achieves delayed initialization, because the CLR does not run static constructors until the type is first accessed (unlike C++ which runs them all when the application loads). In some cases when you have more static methods you might want to prevent the instance from being created when those static methods are accessed; in this case you can put the instance in a nested class.

It meets all the criteria you're after, and avoids complicated easy-to-get-wrong locking patterns.

Thanks, that looks way too easy!

Not at all, the instance is created when the type is initalized. Pretty sure .NET will guarantee threads don't fight over type initialization.

There was a large thread a while ago about creating generic singletons, by the end of it, there was a pretty reasonable method for writing a thread safe singleton, given that you don't mind using the Activator (which is the only way to do a generic singleton), but you should factor in that the singleton will always only have that one instance, so having that instance created by the Activator shouldn't cause problems.

The static method is simple and works.  But sometimes you may need to init the singleton lazy or use a ctor with parms you don't have until after some event.  Here is the way to lazy init a singleton. Actually not much more work then the static method. This also gives you option of poking in another singleton for future callers while letting the last singleton drain.  This may be needed for dynamic runtime config changes without needing to restart the whole app, etc.

public sealed class Singleton
{
    private static Singleton value;
    private static object syncRoot = new Object();
    private Singleton() { }

    public static Singleton Value
    {
        get
        {
            if (Singleton.value == null)
            {
                lock (syncRoot)
                {
                    if (Singleton.value == null)
                    {
                        Singleton newVal = new Singleton();
                        // Insure all writes used to construct new value have been flushed.
                        System.Threading.Thread.MemoryBarrier();
                        // Publish the new object;
                        Singleton.value = newVal;
                    }
                }
            }
            return Singleton.value;
        }
    }
}

staceyw wrote:



The static method is simple and works.  But sometimes you may need to init the singleton lazy or use a ctor with parms you don't have until after some event.  Here is the way to lazy init a singleton. Actually not much more work then the static method. This also gives you option of poking in another singleton for future callers while letting the last singleton drain.  This may be needed for dynamic runtime config changes without needing to restart the whole app, etc.

public sealed class Singleton
{
    private static Singleton value;
    private static object syncRoot = new Object();
    private Singleton() { }

    public static Singleton Value
    {
        get
        {
            if (Singleton.value == null)
            {
                lock (syncRoot)
                {
                    if (Singleton.value == null)
                    {
                        Singleton newVal = new Singleton();
                        // Insure all writes used to construct new value have been flushed.
                        System.Threading.Thread.MemoryBarrier();
                        // Publish the new object;
                        Singleton.value = newVal;
                    }
                }
            }
            return Singleton.value;
        }
    }
}

1.  He already pointed out that the static constructor is done lazily.  No need for this for that reason, though your point about providing construction parameters may hold (though that's a weird concept with singletons... to be valid you'd have to construct with the same parameters every time, which means you might as well put it in with the construction).

2.  This is the DCL (Double-Checked Lock) pattern.  As pointed out already, it's broken under many architectures.  I don't believe the CLR makes any gaurantees in this case, so I wouldn't recommend the pattern.

That all said, I also would discourage the singleton pattern.  It's over used, and is often the wrong solution.  A singleton is a type of global variable, and global variables have all sorts of design issues, especially when writing multi-threaded code.

Use a private method with an attribute to handle the locking to create the instance.

public static Singleton Instance
{
  get
  {
    if (_instance == null)
    {
      InstantiateSingleton();
    }
    return _instance;
  }
}

// requires using System.Runtime.CompilerServices
[MethodImpl(MethodImplOptions.Synchronized)]
private static void InstantiateSingleton()
{
  if (_singleton == null)
  {
    _singleton = new Singleton();
  }
}

wkempf wrote:



staceyw wrote:  The static method is simple and works.  But sometimes you may need to init the singleton lazy or use a ctor with parms you don't have until after some event.  Here is the way to lazy init a singleton. Actually not much more work then the static method. This also gives you option of poking in another singleton for future callers while letting the last singleton drain.  This may be needed for dynamic runtime config changes without needing to restart the whole app, etc. public sealed class Singleton {     private static Singleton value;     private static object syncRoot = new Object();     private Singleton() { }     public static Singleton Value     {         get         {             if (Singleton.value == null)             {                 lock (syncRoot)                 {                     if (Singleton.value == null)                     {                         Singleton newVal = new Singleton();                         // Insure all writes used to construct new value have been flushed.                         System.Threading.Thread.MemoryBarrier();                         // Publish the new object;                         Singleton.value = newVal;                     }                 }             }             return Singleton.value;         }     } }

1.  He already pointed out that the static constructor is done lazily.  No need for this for that reason, though your point about providing construction parameters may hold (though that's a weird concept with singletons... to be valid you'd have to construct with the same parameters every time, which means you might as well put it in with the construction).

2.  This is the DCL (Double-Checked Lock) pattern.  As pointed out already, it's broken under many architectures.  I don't believe the CLR makes any gaurantees in this case, so I wouldn't recommend the pattern.

That all said, I also would discourage the singleton pattern.  It's over used, and is often the wrong solution.  A singleton is a type of global variable, and global variables have all sorts of design issues, especially when writing multi-threaded code.

Mmm, I disagree -

1. The static ctor is only lazy in the sense the singleton only exists when you access its type, but the lazy initialization that was suggested, initializes the singleton when the instance is first called.. the difference in this is that you may have shared methods on the type that don't nec need the singleton instance.

2. I thought double check locking was the only way to safely access objects that may be used by multiple threads.. is it really true that you can't say for sure they work? why do we even bother then?

And I don't really agree with your argument against the singleton, using names like 'global variable' to describe it, makes it sound very ugly, when it isn't like that at all...

stevo_ wrote:

Mmm, I disagree -

1. The static ctor is only lazy in the sense the singleton only exists when you access its type, but the lazy initialization that was suggested, initializes the singleton when the instance is first called.. the difference in this is that you may have shared methods on the type that don't nec need the singleton instance.

2. I thought double check locking was the only way to safely access objects that may be used by multiple threads.. is it really true that you can't say for sure they work? why do we even bother then?

And I don't really agree with your argument against the singleton, using names like 'global variable' to describe it, makes it sound very ugly, when it isn't like that at all...

1.  You can achieve this easy enough, _IF REQUIRED_, by using an internal static class to handle the instantiation of the specific object.

2.  The only way to safely access objects that may be used by multiple threads is by using a synchronization primitive.  DCL isn't a primitive.  It's a poorly designed pattern meant to optimize the synchronization when an object will be read many times but written to only once.  The problem is, this pattern only works on architectures with a specific memory visibility model.  There are architectures that do not have this visibility model, and the DCL pattern leads to a race condition that can cause multiple threads to "initialize" the data, possibly leading to memory corruption or other errors.  If you're writing portable code, or don't have a guarantee about the memory model for your target architecture, you should never use the DCL.  I do not believe the CLR makes any gaurantees here, so you shouldn't use the DCL.  The reality is that, today, on x86 architectures using the Microsoft supplied CLR, the DCL is "safe."  But for all of the reasons I just gave, including the fact that future x86 architectures could well change the memory model, I simply can not recommend anyone use this pattern.  Trust me, you're better off forgetting this pattern exists.  Even the originator of this pattern acknowledges the problems and short comings of it now.

Oh, and I'm sorry, but a Singleton IS a variant on a "global variable".  I'm not saying there's not a place for a Singleton, or any form of "global variable" for that matter.  I'm saying there are real issues that must be considered when you choose to use one, and in general they are used far more often than they need to be, or should be.  Consider alternate designs first.  If the singleton is still your best option, fine.  But that really isn't the case very often.