Singleton code excerpt, an interview-question

Question

I had another interview question. I thought this was silly, but maybe there is something I am missing. The question asked which GoF pattern this is (answer: singleton), and, if there are any problems, how I solve them.

I don't see any problems. I mentioned this is never freed and I expect that from this pattern. That is all I said. Am I missing something?

public class Class1
{
    private static Class1 oInstance = null;
    private Class1() { }
    public static Class1 GetInstance()
    {
        if (oInstance == null)
        {
            oInstance  = new Class1();
        }
        return oInstance ;
    }
}

Answer 1

Thread-safety - multiple instances could be created if GetInstance() is called from competing threads.

Answer 2

You have a potential race condition in multithreaded code. Two threads could each get past the null check before the constructor on the other thread is completed, so that both could end up constructing the class.

Answer 3

There is a potential issue with this if you have a multithreaded application. This can cause more than one instance to be constructed if two threads request at the same time.

I'd take a look at this page on Singletons in C#. It shows the problem in detail, as well as better options.

Answer 4

See: An obvious singleton implementation for .NET?

There are multiple concerns you want to consider when implementing a Singleton pattern.

• What happens when multiple callers request the singleton from multiple threads. It should just work.
• When is the Singleton instance constructor called. You may want to defer it so it happens on the first call requesting the singleton, you may want it first instantiated at some other time.
• Should people be able to inherit off your singleton class? What should the behavior be?
• Should it be possible to switch your singleton instance to a different instance after the singleton has been instantiated. Answering yes to this violates the singleton pattern so in general the field holding singletons should be readonly.
• API design, should you use a property or a method to return the Singleton instance.
• Some people say singletons are evil. Should you even be considering it in the first place. This has been discussed quite often a good starting point is http://blogs.msdn.com/scottdensmore/archive/2004/05/25/140827.aspx

The following is a good general purpose pattern you could follow. Its thread safe, sealed, uses properties and defer instansiates the singleton.

public sealed class Singleton
{
    static class SingletonCreator
    {
        // This may seem odd: read about this at: http://www.yoda.arachsys.com/csharp/beforefieldinit.html
        static SingletonCreator() {}
        internal static readonly Singleton Instance = new Singleton();
    }

    public static Singleton Instance
    {
        get { return SingletonCreator.Instance; }
    }
}

Answer 5

The code is not thread-safe. To make it so you need to do this:

public class Class1
{
    private static Class1 oInstance = null;
    private Class1() { }
    public static Class1 GetInstance()
    {
        if (oInstance == null)
        {
            lock(typeof(Class1))
            {
                if (oInstance == null)
                {
                    oInstance = new Class1();
                }
            }
        }
        return oInstance ;
    }
}

This is the most efficient way of lazily loading the instance as it only bothers to lock (can be expensive) when it is suspected the instance won't be instantiated for the next call. Checking again once in the lock ensures it will only be instantiated once.

Answer 6

so is the solution the following?

public class Class1
{
    private static Class1 oInstance = new Class1();
    private Class1() { }
    public static Class1 GetInstance()
    {
        return oInstance ;
    }
}

Answer 7

Others mentioned thread safety. There's also the fact that they forgot to mark it as sealed, and so you could inherit from it and create multiple instances that way.

Answer 8

Don't work at that company. The singleton pattern isn't a frequently used pattern and it's applicability in a pure OO system is questionable. Undoing a singleton pattern is a huge problem if you ever need to turn it back in to a normally constructed object.

Answer 9

It's missing thread-safety, this page explains it really well.