Stream safe use of singleton elements

Question

I have one single C # class that several classes use. Is access through Instance to the Toggle() method thread safe? If so, under what assumptions, rules, etc. If not, why and how can I fix this?

 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

Access through "Instance" to the class "Toggle ()" threadsafe? If so, under what assumptions, rules, etc. If not, why and how can I fix this?

No, it is not thread safe.

In principle, both threads can run the Toggle function at the same time, so this can 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 work with the following assumption.

If 2 threads work, they can alternate and interact with each other randomly at any point. You can halfway write or read a 64-bit integer or float (on a 32-bit processor), and another thread can jump in and modify it from under you.

If 2 threads never have access to anything at all, it doesn’t matter, but once they do, you should stop them from stepping on each other. The way to do this in .NET is with locks.

You can decide what and where to block by thinking about such things:

For a given block of code, if the value of something has changed from under me, will it matter? If that were the case, you need to block this something for the duration of the code, where it matters.

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; 

To return this value, suppose that value will not change between reading and return . For this assumption to be truly correct, you need to lock value throughout this entire code block.

So you would do this:

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

HOWEVER

In .NET, you can only block link types. Int32 is a value type, so we cannot block it.
We will solve this by introducing the 'dummy' object and blocking it wherever we want to block the "value".

This is what Ben Sheyrman is talking about .

Answer 2

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

An easy way to ensure thread safety is to introduce a lock statement. For example. eg:

 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 3

Here is what I thought. But, I'm looking for details ... 'Toggle ()' is not a static method, but it is a member of a static property (when using "Instance"). Is that what it shares between threads?

If your application is multi-threaded and you can see that several threads will access this method, which makes it common for threads. Since your class is Singleton, you know that another thread will have access to the USED object, so be careful about the thread safety of your methods.

And how this applies to singles in general. Should I call this in every method of my class?

As I said above, because a single singleton, which you know is different from a stream, will have the same object, possibly at the same time. This does not mean that you should force each method to get a lock. If you notice that calling a simultaneous call can lead to a damaged class state, then you should apply the method mentioned by @Thomas

Answer 4

Your thread may stop in the middle of this method and transfer control to another topic. You need a critical section around this code ...

 private static object _lockDummy = new object(); ... lock(_lockDummy) { //do stuff } 

Answer 5

Is it possible to assume that a singleton pattern provides my excellent class, other than threads, for all problems with regular static member threads?

No. Your class is simply not thread safe. Singleton has nothing to do with it.

(I mull over the fact that instance members called by a static object cause threading issues)

This also has nothing to do.

You should think this way: is it possible in my program for 2 (or more) threads to access this part of the data at the same time?

The fact that you receive data through a singleton or static variable or by passing an object as a method parameter does not matter. After all, these are just a few bits and bytes in your PC RAM, and all that matters is whether multiple threads can see the same bits.

Answer 6

I will also add a protected constructor to MyClass so that the compiler does not create a public default constructor.

Answer 7

I thought that if I reset the singleton template and force everyone to get a new instance of the class, this will ease some problems ... but it doesn’t bother anyone from initializing a static object of this type and passing what's around ... or from rotating multiple threads, all access to "Toggle ()" from the same instance.

Bingo :-)

I get it now. This is a complex world. I'm sorry that I am not refactoring outdated code :(

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

Answer 8

Quote:

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

value will always be 0 ...

Answer 9

Well, I really don't know C #, which is good ... but I'm fine in Java, so I will give an answer to this, and hopefully they are similar enough that it will be useful. If not, I apologize.

Answer: no, it is not safe. One thread can call Toggle () at the same time as another, and it is possible, although unlikely with this code, that Thread1 can set a value between when Thread2 checks it and when it installs it.

To fix, just do Toggle () synchronized . It doesn't block anything or name anything that could spawn another thread that could call Toggle (), so all you have to do is save it.