Do I need a lock here?

Question

Do I need to add a lock block here if I want to be sure that instance will be created only 1 time?

        if (instance==null)
        {
            instance = new Class();
        }

Since there is only 1 instruction within the IF I'm not 100% sure. In a case like below, I'm sure I'd need it but I wanted to double check if the same applies for the code above.

        if (instance==null)
        {
            int i = 5;
            int y = 78;
            instance = new Class(i, y);
        }

EDIT

Yes, I assume multithreading

Answer 1

If you are working multithreaded then the answer is YES.

Small note:

Clearly you can't put a lock using instance, because it's null :-)

The Lock paradigm is normally (it's called Double-checked locking):

if (instance == null)
{
    lock (something)
    {
        if (instance == null)
        {
             instance = new Class();
        }
    }
}

If you want (and if creating Class isn't expensive) you can do:

if (instance == null)
{
    Interlocked.CompareExchange(ref instance, new Class(), null);
    // From here you are sure the instance field containts a "Class"
}

The only problem with this piece of code is that two threads could create a new Class(), but only one will be able to set instance with a reference of new Class(), so the other one will have created a useless Class object that will be GC. If creating Class objects is inexpensive (for example creating a List<T>) it's ok. If creating Class objects is expensive (perhaps because the constructor calls a DB an do a big big query) then this method is a no-no.

Small note: the Grinch has arrived and it has declared that all this multi-threading isn't "fool-proof" enough. And he is right. And he is wrong. It's like the Schrödinger's cat! :-) The last time I wrote a multi-threaded program, I lost a week just reading all the literature that there is around the internet. And I still did errors (but ok, I was trying to write lockless MT code... It's quite heavy mumbo jumbo). So use Lazy<T> if you are using .NET 4.0, and if not well... Take the mono source and find where Lazy is defined and copy it :-) (the license is quite permissive, but read it!) BUT If what you need is a singleton, you can use a static Lazy<T> OR if you don't have .NET 4.0 you can use one of the samples from http://www.yoda.arachsys.com/csharp/singleton.html (the fifth one or the fourth one. The author suggest the fourth one. Be aware that there are some interesting caveats on its laziness, but they are written in http://www.yoda.arachsys.com/csharp/beforefieldinit.html). Be aware that you must write them "as written". Don't ever think of deviating from what it's written. DON'T. As you can see reading by the comments, threading is a HARD argument. You can be right and you can still be wrong at the same time. It's like volatile (volatile? Is it a pun?) chemical compounds... Very very funny and very very dangerous.

Another small note: under .NET 1.1 it IS really sticky. You shouldn't share variables between threads in 1.1 unless you know EXACTLY what you are doing. In 2.0 they changed the memory model (how the compiler can optimize access to memory) and they made a "more secure" memory model. .NET 1.1 and older versions of Java (up to the 1.4 included) were a pit trap.

---

To respond to your question, a simple trick: when you are thinking "can MT break my code?" do this: imagine that Windows (the OS) is a lazy ogre. Sometimes he stops a thread for half an hour while letting the other threads run (technically he can do it. Not for 30 minutes (but there aren't real rules on how much long) but for milliseconds it can and it do if the processor is a little overloaded with work and there are many threads pinned to specific processors (pinned means that they told the OS that they want to run ONLY on some specific processors. So if 1000 threads are pinned on processor 1 and processor 2 has to execute only 1 thread that isn't pinned, it's quite clear that the thread on processor 2 will go much more fast!) ). So imagine that two threads enter your piece of code at the same time, execute the first lines at the same time (they are on two different processors, they can be executed REALLY in parallel), but one of the two threads is stopped and has to wait for 30 minutes. What will happen in the mean time? And note that often code can be stopped in the middle of an instruction! a = a + 1 is two instructions! it's var temp = a + 1; a = temp; If you apply this trick to your example code, it's quite easy to see: both threads execute if (instance==null) and pass, one thread is then stopped for 30 minutes, the other one initializes the object, the first one is resumed and it initializes the object. Two objects initialization. No good :-)

I'll explain the .NET 1.1 problem with a simple example:

class MyClass
{
    public bool Initialized;

    public MyClass()
    {
        Initialized = true;
    }
}

MyClass instance = null;

public MyClass GetInstance()
{
    if (instance == null)
    {
        lock (something)
        {
            if (instance == null)
            {
                instance = new Class();
            }
        }
    }

    return instance;
}

Now... This is the code from before. The problem happens in the instance = new Class() line. Let's split it in the various part:

1. Space for the Class object is allocated by .NET. The reference to this space is saved somewhere.
2. The constructors of Class is called. Initialized = true (the field called Initialized!).
3. The instance variable is set to the reference we saved before.

In the newer "strong" memory model of .NET 2.0 these operations will happen in this order. But let's see what could happen uner .NET 1.1: the writes can be reordered!!

1. Space for the Class object is allocated by .NET. The reference to this space is saved somewhere.
2. The instance variable is set to the reference we saved before.
3. The constructors of Class is called. Initialized = true (the field called Initialized!).

Now let's imagine that the thread doing this is suspended by the OS for 30 minutes after point 2 and before point 3. Another thread could access the instance variable (note that the first if in the code isn't protected by a lock, so it can access it without waiting the first thread to end its work) and use it. But Class isn't really initialized: his constructor hasn't run! Boooom! Had you used the volatile keyword on the declaration of instance (so volatile MyClass instance = null;) this wouldn't have happened, because the compiler can't reorder writes past a write on a volatile field. So he can't reorder the point 2 after the point 3 because in point 3 it's writing on a volatile field. But as I have written, this was a problem of .NET 1.1.

Now. if you want to learn about threading, please read this: http://www.albahari.com/threading/ If you want to know what happens "behind the scenes" you could read http://msdn.microsoft.com/en-us/magazine/cc163715.aspx but it's pretty heavy.

Answer 2

If this is multithreaded, then yes, you need a lock or some form of synchronization.

However, if this is to allow lazy instantiation, I'd suggest using Lazy<T>. It handles the thread safety for you, and eliminates the need for the check.

Answer 3

Yes, you need a lock in both of your two examples. Let's number the lines to make the explanation easier:

1 if (instance == null)
2 {
3     instance = new Class();
4 }

Now, assume you have two threads, A and B. Both threads are executing this code. First, A tests instance at line 1, and since it's null it takes the true path - on to line 3. Then, you get a context switch before line 3 is executed and B does the same (true on line 1 and ends up at line 3). Now both threads are inside your if statement body, and you'll get two assignments to instance.

Answer 4

If you're using multiple threads, yes. Multiple threads could enter the if statement before any of them actually set the instance to the new class.

Answer 5

you need to read this article:

Implementing the Singleton Pattern in C#

and this question as well:

Is this Singleton implementation correct and thread-safe?

good luck :)