Thread-safe use of a singleton's members

Question

I have a C# singleton class that multiple classes use. Is access through Instance to the Toggle() method thread-safe? If yes, by what assumptions, rules, etc. If no, why and how can I fix it?

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

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

    private int value = 0;

    public int Toggle()
    {
        if(value == 0) 
        {
            value = 1; 
        }
        else if(value == 1) 
        { 
            value = 0; 
        }

        return value;
    }
}

Answer 1

Is access through 'Instance' to the 'Toggle()' class threadsafe? If yes, by what assumptions, rules, etc. If no, why and how can I fix it?

No, it's not threadsafe.

Basically, both threads can run the Toggle function at the same time, so this could happen

    // thread 1 is running this code
    if(value == 0) 
    {
        value = 1; 
        // RIGHT NOW, thread 2 steps in.
        // It sees value as 1, so runs the other branch, and changes it to 0
        // This causes your method to return 0 even though you actually want 1
    }
    else if(value == 1) 
    { 
        value = 0; 
    }
    return value;

You need to operate with the following assumption.

If 2 threads are running, they can and will interleave and interact with eachother randomly at any point. You can be half way through writing or reading a 64 bit integer or float (on a 32 bit CPU) and another thread can jump in and change it out from underneath you.

If the 2 threads never access anything in common, it doesn't matter, but as soon as they do, you need to prevent them from stepping on each others toes. The way to do this in .NET is with locks.

You can decide what and where to lock by thinking about things like this:

For a given block of code, if the value of something got changed out from underneath me, would it matter? If it would, you need to lock that something for the duration of the code where it would matter.

Looking at your example again

    // we read value here
    if(value == 0) 
    {
        value = 1; 
    }
    else if(value == 1) 
    { 
        value = 0; 
    }
    // and we return it here
    return value;

In order for this to return what we expect it to, we assume that value won't get changed between the read and the return. In order for this assumption to actually be correct, you need to lock value for the duration of that code block.

So you'd do this:

lock( value )
{
     if(value == 0) 
     ... // all your code here
     return value;
}

HOWEVER

In .NET you can only lock Reference Types. Int32 is a Value Type, so we can't lock it.
We solve this by introducing a 'dummy' object, and locking that wherever we'd want to lock 'value'.

This is what Ben Scheirman is referring to.