How to make a static variable thread-safe

Question

I have this static class which contains a static variable (a simple int). I've implemented a lock() in the Run() method of the threads, so no other threads can access to this class concurrently, but the variable still goes crazy, displaying duplicates, insanely high values, etc.

This is the class:

public static class ExplorationManager
{
    public static int Counter = 0;

    public static void ExplorerMaker(List<int[]> validPaths, List<string> myParents, string[,] myExplorationMap, List<int[]> myPositions)
    {
        foreach (var thread in validPaths.Select
        (path => new Explorer(myParents, path, myExplorationMap, myPositions)).
        Select(explorer => new Thread(explorer.Explore)))
            {
                thread.Name = "Thread of " + Counter + " generation";
                Counter++; 
                thread.Start();
    }
}

}

Is there a way to make this variable "more" thread-safe?

Answer 1

There are at least 2 problems that you need to address in order to increase the safety of this type.

The first one is to make Counter private. In it's current form the variable is 100% public and it can be mutated by any piece of code in the application. Today it may be safe but there's nothing protecting you from making a mistake tomorrow. If you still want other pieces of code to be able to read the property then use an accessor

private static int m_counter;
public static int Counter {
  get { return m_counter; }
}

The second problem is that ++ isn't a safe operation on a location that is shared amongst threads. It expands out to the following code

Counter = Counter + 1;

Which is in reality doing

1. load Counter
2. load 1
3. add
4. store Counter

A thread can be interrupted an virtually any time. If one thread is interrupted at step 1, 2 or 3 and another thread fully executes the sequence then you will end up adding / storing stale values. This is why ++ is unsafe. The safe way to increment a shared value amongst threads is to use Interlocked.Increment. It's designed exactly for this purpose

Interlocked.Increment(ref m_counter);

Answer 2

You need to use lock around all reads/writes of your static variable. Something like:

public static readonly object CounterLock = new object();

...
lock ( CounterLock )
{
    Counter++;
}
...

The point is that all reads / writes must be protected by the lock - it's not enough to protect a single place because then threads doing reads or writes may still make a change when a lock elsewhere is in effect.

A lock protects a region of code, not a variable, that's why you need a lock everywhere where you access a shared variable.

Note that you cannot lock on your Counter variable - you need an instance of a reference type as a lock, not a value type. This is why I used object as the lock type (the other answer did the same).