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

Quote:

if(value == 0) { value = 1; }
if(value == 1) { value = 0; }
return value;

value will always be 0...

EDIT: Now it's fixed :)

Answer 2

Your thread could stop in the middle of that method and transfer control to a different thread. You need a critical section around that code...

private static object _lockDummy = new object();


...

lock(_lockDummy)
{
   //do stuff
}

Answer 3

Well, I actually don't know C# that well... but I am ok at Java, so I will give the answer for that, and hopefully the two are similar enough that it will be useful. If not, I apologize.

The answer is, no, it's not safe. One thread could call Toggle() at the same time as the other, and it is possible, although unlikely with this code, that Thread1 could set value in between the times that Thread2 checks it and when it sets it.

To fix, simply make Toggle() synchronized. It doesn't block on anything or call anything that might spawn another thread which could call Toggle(), so that's all you have to do save it.

Answer 4

The original impplementation is not thread safe, as Ben points out

A simple way to make it thread safe is to introduce a lock statement. Eg. like this:

public class MyClass
{
    private Object thisLock = new Object();
    private static readonly MyClass instance = new MyClass();
    public static MyClass Instance
    {
        get { return instance; }
    }
    private Int32 value = 0;
    public Int32 Toggle()
    {
        lock(thisLock)
        {
            if(value == 0) 
            {
                value = 1; 
            }
            else if(value == 1) 
            { 
                value = 0; 
            }
            return value;
        }
    }
}

Answer 5

I'd also add a protected constructor to MyClass to prevent the compiler from generating a public default constructor.

Answer 6

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.

Answer 7

That is what I thought. But, I I'm looking for the details... 'Toggle()' is not a static method, but it is a member of a static property (when using 'Instance'). Is that what makes it shared among threads?

If your application is multi-threaded and you can forsee that multiple thread will access that method, that makes it shared among threads. Because your class is a Singleton you know that the diferent thread will access the SAME object, so be cautioned about the thread-safety of your methods.

And how does this apply to singletons in general. Would I have to address this in every method on my class?

As I said above, because its a singleton you know diferent thread will acess the same object, possibly at the same time. This does not mean you have to make every method obtain a lock. If you notice that a simultaneos invocation can lead to corrupted state of the class, then you should apply the method mentioned by @Thomas

Answer 8

Can I assume that the singleton pattern exposes my otherwise lovely thread-safe class to all the thread problems of regular static members?

No. Your class is simply not threadsafe. The singleton has nothing to do with it.

(I'm getting my head around the fact that instance members called on a static object cause threading problems)

It's nothing to do with that either.

You have to think like this: Is it possible in my program for 2 (or more) threads to access this piece of data at the same time?

The fact that you obtain the data via a singleton, or static variable, or passing in an object as a method parameter doesn't matter. At the end of the day it's all just some bits and bytes in your PC's RAM, and all that matters is whether multiple threads can see the same bits.

Answer 9

@Orion Edwards

I was thinking that if I dump the singleton pattern and force everyone to get a new instance of the class it would ease some problems... but that doesn't stop anyone else from initializing a static object of that type and passing that around... or from spinning off multiple threads, all accessing 'Toggle()' from the same instance.

I get it now. It's a tough world. I wish I weren't refactoring legacy code :(

Answer 10

I was thinking that if I dump the singleton pattern and force everyone to get a new instance of the class it would ease some problems... but that doesn't stop anyone else from initializing a static object of that type and passing that around... or from spinning off multiple threads, all accessing 'Toggle()' from the same instance.

Bingo :-)

I get it now. It's a tough world. I wish I weren't refactoring legacy code :(

Unfortunately, multithreading is hard and you have to be very paranoid about things :-) The simplest solution in this case is to stick with the singleton, and add a lock around the value, like in the examples. Good luck to you.