When Exactly Is A C#/.NET REF Parameter dereferenced? (Looking At Thread Safety)

Question

The question is specifically about when it is safe to invoke ref with a member that is expected to only be read or written while under a monitor lock.

In the example below, the class fields are ONLY expected to be checked and set while under the lock. The private implementations DO ONLY access the values under the lock, BUT, they in fact take in the member as ref, and then lock the lock and do the work on that given ref. The public Get and TrySet methods in fact invoke the private methods by passing the requested member field by ref, and they do NOT lock the lock there at the call site --- and the question was is that actually safe.

It SHOULD be safe, because: though the public methods reference the member field by ref without the lock; at this call site, that ref IS going to ONLY be the pointer; and the actual member value will NOT be dereferenced until in the private method under the required lock.

It would not be safe if:

1. If the public method passing the ref actually reads the value. If so then the private method would receive the value in the argument, and act on that value instead of the current field value; which could have been changed by another thread (and then the logic is broken: the field then is NOT being read under the lock where it is compared, and that has a now stale value).
2. Or if the pointer moved between the public call site and the private method; but I am pretty sure that the CLR ensures no such thing is possible.

Please note that I understand that STILL the returned object ITSELF is not safe: my question is only IN the actual dereferencing of the REF memory.

I have read the spec under 5.1.5; and also looked at the generated IL code; and I believe it is safe.

Here is the example: are the public methods thread safe?

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Threading.Tasks;


namespace Test
{
    public class TestRef
    {
        private readonly object syncLock = new object();
        private ulong eventCounter;
        private object objectValue;
        private long longValue = 1L;


        private T getValue<T>(ref T member)
        {
            lock (syncLock) {
                return member;
            }
        }

        private bool trySetValue<T>(ref T member, T value)
        {
            lock (syncLock) {
                if (object.Equals(member, value))
                    return false;
                member = value;
                ++eventCounter;
            }
            SomeValueChanged?.Invoke(this, EventArgs.Empty);
            return true;
        }


        public object GetObjectValue()
            => getValue(ref objectValue);

        public long GetLongValue()
            => getValue(ref longValue);

        public bool TrySetObjectValue(object value)
            => trySetValue(ref objectValue, value);

        public bool TrySetLongValue(long value)
            => trySetValue(ref longValue, value);


        public ulong EventCounter
        {
            get {
                lock (syncLock) {
                    return eventCounter;
                }
            }
        }

        public event EventHandler SomeValueChanged;
    }


    public static class Program
    {
        public static async Task<bool> ExampleTest(int taskCount)
        {
            TestRef testRef = new TestRef(); // longValue is 1L
            List<Task> tasks = new List<Task>(taskCount);
            for (int i = 0; i < taskCount; ++i) {
                tasks.Add(Task.Run(Callback)); // All Tasks will try set 2L
            }
            await Task.WhenAll(tasks);
            bool success = testRef.EventCounter == 1UL;
            Console.WriteLine(
                    $@"Ran {taskCount} Tasks: Raised event count: {testRef.EventCounter} (success: {success}).");
            return success;
            async Task Callback()
            {
                await Task.Delay(taskCount); // Cheaply try to pile Tasks on top of each other
                testRef.TrySetLongValue(2L);
            }

            // If not safe, then it is possible for MORE THAN ONE
            // Task to raise the event: i.e. two may
            // begin and the public method could read the
            // current value outside the lock, and both
            // would read 1L; and then BOTH would compare
            // the argument in the private method AS 1L
            // and both would then set the value and raise the event.
            // If safe, then only the first Task in would change
            // the value
        }

        public static void Main(string[] args)
        {
            int defaultTaskCount = Environment.ProcessorCount * 500;
            Console.WriteLine($@"Hello World.");
            Console.WriteLine(
                    $@"Specify how many parallel Tasks to run against {Environment.ProcessorCount} instances (each):");
            Console.WriteLine(
                    $@"--- The default will be {
                                defaultTaskCount
                            } Tasks against each instance [just type enter for the default]:");
            if (!int.TryParse(Console.ReadLine(), NumberStyles.Any, CultureInfo.CurrentCulture, out int taskCount))
                taskCount = defaultTaskCount;
            Console.WriteLine($@"Will Run {taskCount} Tasks against {Environment.ProcessorCount} instances (each) ...");
            List<Task<bool>> tasks = new List<Task<bool>>(Environment.ProcessorCount);
            for (int i = 0; i < Environment.ProcessorCount; ++i) {
                tasks.Add(Program.ExampleTest(taskCount));
            }
            Task.WhenAll(tasks)
                    .Wait();
            bool success = tasks.All(task => task.Result);
            Console.WriteLine($@"Success = {success}.");
            Console.WriteLine($@"Type a key to exit ...");
            Console.ReadKey();
        }
    }
}

The public methods do not lock the lock, and pass the member by reference; and the private methods lock the lock before reading and writing.

I am under the assumption that it IS safe: the passed reference is ONLY the pointer at the call site; and the private method bodies actually dereference the pointer; under the lock there.

In the generated IL code, IT APPEARS to be safe: only the pointer is passed, and that is not dereferenced until under the lock in the private method.

The spec DOES say that "Within a function member or anonymous function, a reference parameter is considered initially assigned." --- But it says "CONSIDERED initially assigned" ... which might add more question, but leads me to think that the pointer is not deferenced until it is used, and so the above IS always safe.

Answer 1

Since you are Getting and Setting the value from outside your class this will not ensure thread safety at all. And with an object the reference is getting changed not the value, which means that each thread that sets the value back to your Test class will be changing the reference. The thread that retrieved the reference prior to this will be working on the original reference, not the modified one.

With the code below you can see not only are the numbers out of order, but Seven and Ten are missing because the threads are all in a race condition and your lock isn't blocking any of the logic.

var test = new Test();
test.SetValue("");

var newStrings = new string[] { "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine", "Ten" };

newStrings.AsParallel()
    .ForAll(newString =>
    {
        var value = test.GetValue() as string;
        value += $" {newString}";
        test.SetValue(value);
    });

// Eight, Six, Three, Four, One, Two, Five, Nine
var result = test.GetValue() as string;

If you want thread safety you need to be locking the value while your threads are changing it.

With the improved code below, you will notice that now you have all the words added, but they are still out of order.

When dealing with thread safety you really need to step back and look at the whole picture as not one place with a couple locks is going to magically make an entire class or application thread safe.

public class Test<T> where T : class
{
    private readonly object syncLock = new object();
    private T _value;

    public delegate void MyAction(ref T value);

    public void EditObject(MyAction action)
    {
        lock (syncLock)
        {
            action(ref _value);
        }
    }

    public T GetValue()
    {
        lock (syncLock)
        {
            return _value;
        }
    }
}

var test = new Test<string>();

var newStrings = new string[] { "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine", "Ten" };

newStrings.AsParallel()
    .ForAll(newString =>
    {
        test.EditObject((ref string o) => o += $" {newString}");
    });

// Three Four Five Six One Two Ten Nine Eight Seven
var result = test.GetValue() as string;

Answer 2

I wrote this little sample program to demonstrate what I mean about ref being dereferenced every time it is used. I hope this helps.

As far as thread safety is concerned, when ref gets dereferenced has little effect. Your code needs to ensure that no two threads are modifying the value at the same time, and that no threads are reading the value until the writing threads have completed.

Your new code is much improved. But remember, it is the locks that are making your code thread safe, not when ref is dereferenced.

class Program
{
    static void Main(string[] args)
    {
        var a = new Test { Name = "First" };

        ref Test b = ref a;
        ref Test c = ref a;

        Console.WriteLine(b.Name); // dereferences b, prints "Hello World!"
        Console.WriteLine(c.Name); // dereferences c, prints "Hello World!"

        b = new Test { Name = "Goodbye :(" }; // change the target of ref b to a new object

        // dereference c again, points to the new object
        // prints "Goodbye :("
        Console.Write(c.Name); 
    }
}

public class Test
{
    public string Name { get; set; }
}