Thread Safe Properties in C#

Question

I am trying to create thread safe properties in C# and I want to make sure that I am on the correct path - here is what I have done -

private readonly object AvgBuyPriceLocker = new object();
private double _AvgBuyPrice;
private double AvgBuyPrice 
{
    get
    {
        lock (AvgBuyPriceLocker)
        {
            return _AvgBuyPrice;
        }
    }
    set
    {
        lock (AvgBuyPriceLocker)
        {
            _AvgBuyPrice = value;
        }
    }
}

Reading this posting, it would seem as if this isn't the correct way of doing it -

C# thread safety with get/set

however, this article seems to suggest otherwise,

http://www.codeproject.com/KB/cs/Synchronized.aspx

Does anybody have a more definitive answer?

Edit:

The reason that I want to do the Getter/Setter for this property is b/c I actually want it to fire an event when it is set - so the code would actually be like this -

public class PLTracker
{

    public PLEvents Events;

    private readonly object AvgBuyPriceLocker = new object();
    private double _AvgBuyPrice;
    private double AvgBuyPrice 
    {
        get
        {
            lock (AvgBuyPriceLocker)
            {
                return _AvgBuyPrice;
            }
        }
        set
        {
            lock (AvgBuyPriceLocker)
            {
                Events.AvgBuyPriceUpdate(value);
                _AvgBuyPrice = value;
            }
        }
    }
}

public class PLEvents
{
    public delegate void PLUpdateHandler(double Update);
    public event PLUpdateHandler AvgBuyPriceUpdateListener;

    public void AvgBuyPriceUpdate(double AvgBuyPrice)
    {
        lock (this)
        {
            try
            {
                if (AvgBuyPriceUpdateListener!= null)
                {
                    AvgBuyPriceUpdateListener(AvgBuyPrice);
                }
                else
                {
                    throw new Exception("AvgBuyPriceUpdateListener is null");
                }
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
        }
    }
}

I am pretty new to making my code thread safe so please feel free to tell me if I am going about it in the totally wrong way!

Will

Solution 1

Since you have a primitive value this locking will work fine - the issue in the other question was that the property value was a more complex class (a mutable reference type) - the locking will protect accessing and retrieving the instance of the double value held by your class.

If your property value is a mutable reference type on the other hand locking will not protect from changing the class instance once retrieved using its methods, which is what the other poster wanted it to do.

Solution 2

The locks, as you have written them are pointless. The thread reading the variable, for example, will:

1. Acquire the lock.
2. Read the value.
3. Release the lock.
4. Use the read value somehow.

There is nothing to stop another thread from modifying the value after step 3. As variable access in .NET is atomic (see caveat below), the lock is not actually achieving much here: merely adding an overhead. Contrast with the unlocked example:

1. Read the value.
2. Use the read value somehow.

Another thread may alter the value between step 1 and 2 and this is no different to the locked example.

If you want to ensure state does not change when you are doing some processing, you must read the value and do the processing using that value within the contex of the lock:

1. Acquire the lock.
2. Read the value.
3. Use the read value somehow.
4. Release the lock.

Having said that, there are cases when you need to lock when accessing a variable. These are usually due to reasons with the underlying processor: a double variable cannot be read or written as a single instruction on a 32 bit machine, for example, so you must lock (or use an alternative strategy) to ensure a corrupt value is not read.

Solution 3

Thread safety is not something you should add to your variables, it is something you should add to your "logic". If you add locks to all your variables, your code will still not necessarily be thread safe, but it will be slow as hell. To write a thread-safe program, Look at your code and decide where multiple threads could be using the same data/objects. Add locks or other safety measures to all those critical places.

For instance, assuming the following bit of pseudo code:

void updateAvgBuyPrice()
{
    float oldPrice = AvgBuyPrice;
    float newPrice = oldPrice + <Some other logic here>
    //Some more new price calculation here
    AvgBuyPrice = newPrice;
}

If this code is called from multiple threads at the same time, your locking logic has no use. Imagine thread A getting AvgBuyPrice and doing some calculations. Now before it is done, thread B is also getting the AvgBuyPrice and starting calculations. Thread A in the meantime is done and will assign the new value to AvgBuyPrice. However, just moments later, it will be overwritten by thread B (which still used the old value) and the work of thread A has been lost completely.

So how do you fix this? If we were to use locks (which would be the ugliest and slowest solution, but the easiest if you're just starting with multithreading), we need to put all the logic which changes AvgBuyPrice in locks:

void updateAvgBuyPrice()
{
    lock(AvgBuyPriceLocker)
    {
        float oldPrice = AvgBuyPrice;
        float newPrice = oldPrice + <Some other code here>
        //Some more new price calculation here
        AvgBuyPrice = newPrice;
    }
}

Now, if thread B wants to do the calculations while thread A is still busy, it will wait until thread A is done and then do its work using the new value. Keep in mind though, that any other code that also modifies AvgBuyPrice should also lock AvgBuyPriceLocker while it's working!

Still, this will be slow if used often. Locks are expensive and there are a lot of other mechanism to avoid locks, just search for lock-free algorithms.

Solution 4

Reading and writing of doubles is atomic anyway (source) reading and writing of doubles isn't atomic and so it would be necessary to protect access to a double using a lock, however for many types reading and writing is atomic and so the following would be just as safe:

private float AvgBuyPrice
{
    get;
    set;
}

My point is that thread safety is more complex than simply protecting each of your properties. To give a simple example suppose I have two properties AvgBuyPrice and StringAvgBuyPrice:

private string StringAvgBuyPrice { get; set; }
private float AvgBuyPrice { get; set; }

And suppose I update the average buy price thusly:

this.AvgBuyPrice = value;
this.StringAvgBuyPrice = value.ToString();

This clearly isn't thread safe and individually protecting properties in the above way won't help at all. In this case the locking should be performed at a different level rather than at a per-property level.

View more solutions

Comments

[JeremyDWill]

According to your MSDN source, doubles are not guaranteed to be atomic: "Reads and writes of other types, including long, ulong, double, and decimal, as well as user-defined types, are not guaranteed to be atomic."

[Justin]

@JeremyDWill Well spotted - I've edited my question, however I think my point still stands.

[as-cii]

I agree with JeremyDWill, you should consider that double is a 64 bit number so when working with 32 bit machines read and write operations could be done in more than one step.

[Marcel Gosselin]

To add to Mart's comment, Joseph Albahari has written an excellent online book regarding all threading aspects. The book is available as HTML or PDF.

[JMarsch]

It's never safe to read/write without a memory fence. If you aren't locking, then you would need to declare your property variable as volatile or use a memory fence. Do some searching on Joe Duffy's blog for more info.

[Theodor Zoulias]

This solution is a potentially better performing implementation of a bad idea. Mart's answer explains why protecting individual reads/writes of properties is pointless in most cases.

[Codigo]

@theodor-zoulias As you have correctly pointed out, in MOST cases it is pointless. However, the question is asking exactly about the situation in which it DOES make sense to the person asking the question. I try just to provide a better solution for SUCH a case, since Mart has pointed to the remaining directions.

[Theodor Zoulias]

The OP is admitting in their question that they are pretty new to thread-safety and multithreading, and judging from the code in the question they don't really know what they are doing. They are asking for advices about how to do make what they intend to do correctly, not about how to optimize their existing code so that it produces incorrect results faster.

[Codigo]

@theodor-zoulias the results of my proposed code are correct, since we are using primitive types. Your comment is incorrect, since my code provides a correct and quite optimal solution for the code in the question. Actually, my code works quite fine in all cases with primitive types, when the costs of the lock are not too expensive, which is probably the majority of all possible scenarios.

[Theodor Zoulias]

Codigo your code is no more correct than OP's code, and OP's code is incorrect in that it applies thread-safety to the wrong level of abstraction. It applies it to individual properties instead of applying it to the complex operations that are required to be atomic in a specific application. The mere fact that they want to add thread-safety around events should be telling enough. There is no built-in class in the .NET platform that is thread-safe and exposes events. Events are irreparably thread-unsafe in .NET.

[Codigo]

@theodor-zoulias Ok, let us discus further in a friendly way: you are most likely mixing up 2 different things: (i) thread safe properties, which is the subject of this question and (ii) having the "latest" value in a property, even when updated by event handlers. To make a property with primitive type thread safe (as required in this question), my proposed code is correct. It does exactly that. When the "latest" property value is required, then and only then you need to synchronize the values on a higher lever of abstraction. But, such a scenario is not the subject of this question.

[Theodor Zoulias]

Codigo the OP already knows how to make thread-safe properties. Using a ReaderWriterLockSlim instead of a lock doesn't make them more thread-safe, only (potentially) more efficient. In this regard your answer is off topic IMHO. You neither confirmed nor refuted that their way of doing it is correct, which is what the OP asked. The point raised by Mart and others is that thread-safe properties are pointless. We don't know what the OP is going to do with their thread-safe properties, but chances are that it's not going to be a robust program that produces correct results consistently.