.Net singleton pattern multi-threading issue…


I came across this today in some code i am reviewing ... and thought i would share it with everyone as i am sure not many people know about this...


In the singleton pattern you have a static member variable that 'stores' your single instance of whatever you are giving out.


Most code that i review does it like this:


E.g.


public class Yada
{
    private static MyFunkyObject _foo;


    public static MyFunkyObject Foo
    {


        get
        {


           if(_foo == null)
              _foo = new MyFunkyObject();

           return _foo;
        }
    }
}


Now ... this wont work.  Why?  Because it is not threadsafe.  You 'could' potentially have two threads come in and both get past the if(_foo == null) statement ... and then both go and set _foo up.


This breaks your singleton pattern. Most coders who i point this out to don't even realize they have this issue.


To make your singleton threadsafe you need to do a couple of things.  Put a Mutex or Critical section around access to _foo.  In C# you have the luxury of using the 'lock' keyword to do this.  This gives you a nice threadsafe singleton ... but your code takes a performance hit.


E.g.


public class Yada
{
    private static MyFunkyObject _foo;
    pr
ivate Object _synclock = new Object();


    public static MyFunkyObject Foo
    {


        get
        {

            lock(_synclock)
            {
                 if(_foo == null)
                     _foo = new MyFunkyObject();
            }


            return _foo;
        }
    }
}


There is an even nicer way to do this:


public class Yada
{
    private static MyFunkyObject _foo = new MyFunkyObject();


    public static string Foo
    {
        get
        {
            return _foo;
        }
    }
}


I have seen many peoples Singleton implementations coded poorly as per the first example.  Maybe most people are developing and testing on a single proc machine and don't see this happen ... but you could quite easily be having these issues when you deploy onto that nice beefy 4 proc web server.


(I think i have got this right ... but if please correct me if i am wrong!)

Comments (15)

  1. dsuspense says:

    Where have you been. This problem has been around for years. It has especially been a big problem when implementing clustered J2EE systems that use singletons. Luckily as you stated, C# has the ‘lock’ keywork, which makes the workaround alot easier 🙂

  2. Chris Johnson says:

    I know this problem has been around for a long time … and that is why i am amazed at the number of times i see it in customers code 🙂

  3. Fabrice says:

    Even if it’s been around for years, it’s good to remind people of the potential hazard.

    This is something that hadn’t come to my mind.

  4. Andy Smith says:

    This is the implementation recommended on msdn:

    public class Foo {

    private static Foo _theFoo = new Foo();

    public static Foo TheFoo {

    get {

    return _theFoo;

    }

    }

    And, in a think-different kinda way… it seems to me that there’s no practical difference between these implementations, and just using a class with all static members.

  5. AT says:

    C# is not a clone of Java – you can not reuse same patterns blindly.

    A must to read for everybody using Singeltons in C# – http://www.yoda.arachsys.com/csharp/singleton.html

    Beware – there is can static members in class that do not requere instantion of singelton. So simply all statics members will durn your memory and slow down app start-up.

    The best one with lazy instantion is:

    public class Foo

    {

    //sealed can be replaced to static in C# 2.0 ??

    sealed class FooTheFooSingelton

    {

    internal static readonly Foo fooInstance = new Foo();

    static FooTheFooSingelton() { }

    }

    public static Foo TheFoo

    {

    get

    {

    return FooTheFooSingelton.fooInstance;

    }

    }

    }

    This will shift locking from your code to CLR runtime

  6. Kazuhiko Kikuchi says:

    >Beware – there is can static members in class that do not requere instantion of singelton. So simply all statics members will durn your memory and slow down app start-up.

    No.

    static instance initialization runs on first access to class.

  7. Chris,

    Great job of pointing out the issue. Not many developers that I know using Patterns, but when they do post like yours help them out to write correct code.

    Maxim

    By the way, It is recommend that you create an instance on the object always upon its declaration.

    [www.ipattern.com do you?]

  8. Boris Mühmer says:

    recently there was a "blogs.msdn.com" entry with the following msdn link about .Net singletons:

    "Exploring the Singleton Design Pattern"

    http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnbda/html/singletondespatt.asp

  9. AT says:

    Kazuhiko:

    I mean that your Yada class can have static DoMagic() which do not require your Singleton.

    If DoMagic() called during application startup this result in useless instance of singleton created and will increase your memory footprint and start time.

  10. AT says:

    Chris.

    Your second example with locking will not work correctly without Thread.MemoryBarrier() and may return not completely created version of MyFunkyObject to others CPUs becouse of memory write-reordering and caches filled with outdated data.

    Better check this long discussion to find correct answers:

    http://blogs.msdn.com/brada/archive/2004/05/12/130935.aspx

    IMHO, The best is to to still use static field initializer in a nested type if your class have more that one static method. Or use your 3-rd example for trivial cases then you need have only one static method (get_Foo property) and your class is _sealed_ (something that is not true in your example). Subclasses constructors will call you default (or any other) contructor and useless singleton instances will be created.

    Your 3rd example possibly have some performance benefits for active use (compared to nested type) but tradeoff is creation of instance too early.

  11. Daniel Stolt says:

    I’d like to make two comments. First, you don’t have to declare a dedicated object to lock() on, just because you are doing it in static code. You can lock() on the Type of your class, like this:

    lock(typeof(Yada))

    {

    // thread-safe code here…

    }

    Also, I just thought I’d point out that there is of course also the option of using a static constructor for thread-safe initialization of static members, like so:

    public class Yada

    {

    private static MyFunkyObject _foo;

    static Yada()

    {

    _foo = new MyFunkyObject();

    }

    public static MyFunkyObject Foo

    {

    get

    {

    return _foo;

    }

    }

    }

  12. Fabrice says:

    Daniel, this excerpt from the documentation explains why lock(typeof(MyType)) is not a good idea:

    In multithreading scenarios, do not lock Type objects in order to synchronize access to static data. Other code, over which you have no control, might also lock your class type. This might result in a deadlock. Intead, synchronize access to static data by locking a private static object.

  13. Ashish Das says:

    Chris,

    I came across your posting via one of the suggestions posted on NG m.p.dotnet.language.csharp. It is amazing that your code is referred by many and still none could point that the following code is wrong (and will not compile) as it is returning ‘string’ rather MyFunkyObject.

    public class Yada

    {

    private static MyFunkyObject _foo = new MyFunkyObject();

    public static string Foo <- return string

    {

    get

    {

    return _foo; <– return object not string

    }

    }

    }

  14. Deepak says:

    Make sure you mark the class as sealed and include a private constructor.

Skip to main content