1

Recently I found this article that explains how to implement a thread safe singleton pattern in C#. However I wonder how important it is to make your singleton thread safe what and what are the dangers of having your singleton class non thread safe. Consider a very simple, non thread safe, implementation of a singleton pattern:

public class ThreadNotSafeSingleton
{
    public static ThreadNotSafeSingleton Instance
    {
        get
        {
            if (_instance == null)
            {
                _instance = new ThreadNotSafeSingleton();
            }

            return _instance;
        }
    }

    private static ThreadNotSafeSingleton _instance;

    private ThreadNotSafeSingleton()
    {
    }
}

Now let's say, due to the fact that this code is non thread safe, two threads will enter this line of code _instance = new ThreadNotSafeSingleton();. In this case, the first thread will initialize _instance field and then the second one will initialize _instance field again, any way as a result you will have a single _instance existing in your application.

So what is the big deal here then? I know, if you have more complex code, and your constructor runs some other code that may not be thread safe it may be dangerous. But are there any problems with non thread safety if your singleton is that simple?

Mykhailo Seniutovych
  • 3,527
  • 4
  • 28
  • 50
  • 2
    Use `Lazy`. See the second answer to [this question](https://stackoverflow.com/questions/6847721/when-should-i-use-lazyt)..The cost and difficulty of troubleshooting a production threading issue is typically very high, and non-reproducible elsewhere, so don't cut corners on this sort of thing. – John Wu Jun 22 '19 at 09:07
  • @JohnWu You are right. But `Lazy` guarantees thread safe _construction_ only. So keep in mind that the returned object (the singleton instance) itself is still not thread safe. You might still need to add synchronization to the singleton class if you want the instance to be tread safe. – BionicCode Jun 22 '19 at 09:37
  • I would personally recommend using https://stackoverflow.com/questions/34529533/system-lazyt-with-different-thread-safety-mode/42567351#42567351 to get the benefits of `Lazy` without its default exception caching behaviour. – mjwills Jun 22 '19 at 12:25
  • 1
    `any way as a result you will have a single _instance existing in your application.` That is not true. The first call may have returned the first instance and the second call may have returned the second instance (and those instances may be passed around to other code, or stored in member variables - who knows where they are, they may be used for hours to come). Thus there is a strong possibility that you **don't** have a single instance. You have two of them. – mjwills Jun 22 '19 at 12:27

2 Answers2

3

Since the singleton pattern restricts the class to only one instance, your approach violates that pattern.

If for any reason this is ok for your implementation, and we follow your stated example, there is a very high chance that each of the two concurrent accesses will get another instance of ThreadNotSafeSingleton. This may happen due to optimization if the compiler decides that there is no need to read back the _instance variable it just wrote before returning. This optimization behaviour is defined by the memory model of your C# implementation.

The volatile keyword is often cited as a possible solution, but it will not solve the synchronization issue (as pointed out by BionicCode), when thread 1 passes the if (_instance == null)line and gets put to sleep, then thread 2 also evaluates the same if and gets to instanciate the singleton. When thread 1 wakes up later, it will then instanciate another singleton.

BionicCode
  • 1
  • 4
  • 28
  • 44
user1781290
  • 2,674
  • 22
  • 26
  • `volatile` alone doesn't solve the problem. `volatile` gives guarantees that the changed value is immediately visible to all threads. This is a concurrency problem and therefore introduces a race condition. E.g. the statement `if (x == null)` consists at least of two instructions: read `x` from memory, compare to NULL, return result. Between each instruction the OS can switch the thread. So `volatile` doesn't help. If we passed the null check but then a context switch occurred and the next instruction is `x = new Object()` we still introducing side effects. Race condition. – BionicCode Jun 22 '19 at 09:27
  • The answer to the problem is synchronization. And because the if-statement is split up internally, double checking pattern is recommended when acquiring a synchronization lock. – BionicCode Jun 22 '19 at 09:27
  • That's why I wrote that you can solve SOME of the issues with volatile. Maybe cutting it to only one sentence was too short – user1781290 Jun 22 '19 at 09:29
  • It will solve nothing. The problem still exists. I can solve the problem using synchronization alone, without `volatile`. – BionicCode Jun 22 '19 at 09:31
3

The purpose of the Singleton pattern is to create just and only just one object from a Type.

Imagine thread1 and thread2 come to the get accessor of the Instance property for the first time (_instance is still null) at the same time:

thread1 checks the if (_instance == null) expression and sees that _instance is null, and at the same time, thread2 checks the if expression and sees that _instance is null yet too. thread1 comes to this _instance = new ThreadNotSafeSingleton(); expression and creates a new object from the ThreadNotSafeSingleton type. Because thread2 checks the if expression and sees that _instance is null too, comes to the if code block and creates a new object that will overwrite the reference of the _instance variable. Finally, thread1 uses an object that is different from thread2's object.

This problem occurs when _instance is null and two or more threads try to get an instance of your Type at the same time. You must restrict simultaneous access to the if code body for threads (thread synchronization), especially when your _instace is null yet.