Partager via

.Net singleton pattern multi-threading issue...

  • Article

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;
private 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

  • Anonymous
    June 10, 2004
    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 :-)

  • Anonymous
    June 10, 2004
    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 :)

  • Anonymous
    June 10, 2004
    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.

  • Anonymous
    June 10, 2004
    The comment has been removed

  • Anonymous
    June 10, 2004
    The comment has been removed

  • Anonymous
    June 10, 2004
    >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.

  • Anonymous
    June 10, 2004
    The comment has been removed

  • Anonymous
    June 10, 2004
    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

  • Anonymous
    June 10, 2004
    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.

  • Anonymous
    June 10, 2004
    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.

  • Anonymous
    June 12, 2004
    The comment has been removed

  • Anonymous
    June 14, 2004
    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.

  • Anonymous
    July 26, 2005
    The comment has been removed

  • Anonymous
    September 19, 2007
    Make sure you mark the class as sealed and include a private constructor.

  • Anonymous
    February 12, 2008
    PingBack from http://briannoyle.wordpress.com/2008/02/12/multi-threading-musings/