Does lock section always guarantee thread safety?

Question

I'm trying to understand thread-safe access to fields. For this, i implemented some test sample:

class Program
{   
    public static void Main()
    {
        Foo test = new Foo();
        bool temp;

        new Thread(() => { test.Loop = false; }).Start();

        do
        {
            temp = test.Loop;
        }
        while (temp == true);
    }
}

class Foo
{
    public bool Loop = true;
}

As expected, sometimes it doesn't terminate. I know that this issue can be solved either with volatile keyword or with lock. I consider that i'm not author of class Foo, so i can't make field volatile. I tried using lock:

public static void Main()
{
    Foo test = new Foo();
    object locker = new Object();
    bool temp;

    new Thread(() => { test.Loop = false; }).Start();

    do
    {
        lock (locker)
        {
            temp = test.Loop;
        }
    }
    while (temp == true);
}

this seems to solve the issue. Just to be sure i moved the cycle inside the lock block:

lock(locker)
{
    do
    {
        temp = test.Loop;
    }
    while (temp == true);
}

and... the program does not terminates anymore.

It is totally confusing me. Doesn't lock provides thread-safe access? If not, how to access non-volatile fields safely? I could use VolatileRead(), but it is not suitable for any case, like not primitive type or properties. I considered that Monitor.Enter does the job, Am i right? I don't understand how could it work.

Answer 1

This piece of code:

do
{
    lock (locker)
    {
        temp = test.Loop;
    }
}
while (temp == true);

works because of a side-effect of lock: it causes a 'memory-fence'. The actual locking is irrelevant here. Equivalent code:

do
{
   Thread.MemoryBarrier();   
   temp = test.Loop;       
}
while (temp == true);

And the issue you're trying to solve here is not exactly thread-safety, it is about caching of the variable (stale data).

Answer 2

It does not terminate anymore because you are accessing the variable outside of the lock as well.

In

new Thread(() => { test.Loop = false; }).Start();

you write to the variable outside the lock. This write is not guaranteed to be visible.

Two concurrent accesses to the same location of which at least one is a write is a data race. Don't do that.

Answer 3

Lock provides thread safety for 2 or more code blocks on different threads, that uses the lock. Your Loop assignment inside the new thread declaration is not enclosed in lock. That means there is no thread safety there.

Answer 4

In general, no, lock is not something that will magically make all code inside it thread-safe.

The simple rule is: If you have some data that's shared by multiple threads, but you always access it only inside a lock (using the same lock object), then that access is thread-safe.

Once you leave that “simple” code and start asking questions like “How could I use volatile/VolatileRed() safely here?” or “Why does this code that doesn't use lock properly seem to work?”, things get complicated quickly. And you should probably avoid that, unless you're prepared to spend a lot of time learning about the C# memory model. And even then, bugs that manifest only once in million runs or only on certain CPUs (ARM) are very easy to make.

Answer 5

Locking only works when all access to the field is controlled by a lock. In your example only the reading is locked, but since the writing is not, there is no thread-safety.

However it is also crucial that the locking takes place on a shared object, otherwise there is no way for another thread to know that someone is trying to access the field. So in your case when locking on an object which is only scoped inside the Main method, any other call on another thread, would not be able to block.

If you have no way to change Foo, the only way to obtain thread-safety is to have ALL calls actually lock on the same Foo instance. This would generally not be recommended though, since all methods on the object would be locked.

The volatile keyword is not a guarantuee of thread-safety in itself. It is meant to indicate that the value of a field can be changed from different threads, and so any thread reading that field, should not cache it, since the value could change.

To achieve thread-safety, Foo should probably look something along these lines:

class Program
{   
    public static void Main()
    {
        Foo test = new Foo();
        test.Run();

        new Thread(() => { test.Loop = false; }).Start();

        do
        {            
            temp = test.Loop;
        }
        while (temp == true);
    }
}

class Foo
{
    private volatile bool _loop = true;
    private object _syncRoot = new object();

    public bool Loop
    {
        // All access to the Loop value, is controlled by a lock on an instance-scoped object. I.e. when one thread accesses the value, all other threads are blocked.
        get { lock(_syncRoot) return _loop; }
        set { lock(_syncRoot) _loop = value; }
    }

    public void Run()
    {
        Task(() => 
        {
            while(_loop) // _loop is volatile, so value is not cached
            {
                // Do something
            }
        });
    }
}