What are the Dangers of using a Singleton in a multithreaded application

Question

I'm looking at using a singleton in a multithreaded Win service for doing logging, and wanted to know what are some of the problems I might encounter. I have already set up the get instance to handle syncing with

    private static volatile Logging _instance;
    private static object _syncRoot = new object();

    private Logging(){}
    public static Logging Instance
    {
        get
        {
            if (_instance==null)
            {
                lock(_syncRoot)
                {
                    if (_instance == null)
                    {
                        _instance = new Logging();
                    }
                }
            }
            return _instance;
        }
    }

Is there anything else I might need to worry about?

Answer 1

That looks pretty good to me.

See this for more info.

edit: Should probably put the return inside the lock, though.

Answer 2

I think if Logging instance methods are thread-safe there's nothing to worry about.

Answer 3

Singleton's have the potential to become a bottleneck for access to the resource embodied by the class, and force sequential access to a resource that could otherwise be used in parallel.

In this case, that may not be a bad thing, because you don't want multiple items writing to your file at the same instant, and even so I don't think your implementation will have that result. But it's something to be aware of.

Answer 4

You are using double-checked locking what is considered a anti-pattern. Wikipedia has patterns with and without lazy initialization for different languages.

After creating the singleton instance you must of course ensure that all methods are thread-safe.

Answer 5

This is more informational than anything else.

What you've posted is the double-checked locking algorithm - and what you've posted will work, as far as I'm aware. (As of Java 1.5 it works there, too.) However, it's very fragile - if you get any bit of it wrong, you could introduce very subtle race conditions.

I usually prefer to initialize the singleton in the static initializer:

public class Singleton
{
    private static readonly Singleton instance = new Singleton();

    public static Singleton Instance
    {
        get { return instance; }
    }

    private Singleton()
    {
        // Do stuff
    }
}

(Add a static constructor if you want a bit of extra laziness.)

That pattern's easier to get right, and in most cases it does just as well.

There's more detail on my C# singleton implementation page (also linked by Michael).

As for the dangers - I'd say the biggest problem is that you lose testability. Probably not too bad for logging.

Answer 6

A better suggestion would be to establish the logger in a single-threaded setup step, so it's guaranteed to be there when you need it. In a Windows Service, OnStart is a great place to do this.

Another option you have is to used the System.Threading.Interlocked.CompareExchange(T%, T, T) : T method to switch out. It's less confusing and it's guaranteed to work.

System.Threading.Interlocked.CompareExchange<Logging>(_instance, null, new Logging());

Answer 7

You need to ensure that each method in the logger are safe to run concurrently, i.e. that they don't write to shared state without proper locking.

Answer 8

There is some debate with respect to the need to make the first check for null use Thread.VolatileRead() if you use the double checked locking pattern and want it to work on all memory models. An example of the debate can be read at http://social.msdn.microsoft.com/forums/en-US/csharpgeneral/thread/b1932d46-877f-41f1-bb9d-b4992f29cedc/.

That said, I typically use Jon Skeet's solution from above.