CODEWITHSUNDEEP
c#.netmultithreadingsingleton
Bob The Janitor
Bob The Janitor

Reputation: 20802

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

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?

Upvotes: 9

Views: 3042

Answers (8)

Jon Skeet
Jon Skeet

Reputation: 1503479

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.

Upvotes: 12

Michael Todd
Michael Todd

Reputation: 17071

That looks pretty good to me.

See Implementing the Singleton Pattern in C# for more info.

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

Upvotes: 13

Rob Scott
Rob Scott

Reputation: 449

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.

Upvotes: 1

Cristian Libardo
Cristian Libardo

Reputation: 9258

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.

Upvotes: 2

Joel Coehoorn
Joel Coehoorn

Reputation: 416131

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.

Upvotes: 3

C. Ross
C. Ross

Reputation: 31878

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());

Upvotes: 1

Daniel Br&#252;ckner
Daniel Br&#252;ckner

Reputation: 59705

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.

Upvotes: 1

Dmitry Ornatsky
Dmitry Ornatsky

Reputation: 2237

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

Upvotes: 0

Related Questions