c# - Volatile keyword usage vs lock

Question

I've used volatile where I'm not sure it is necessary. I was pretty sure a lock would be overkill in my situation. Reading this thread (Eric Lippert comment) make me anxious on my usage of volatile: When should the volatile keyword be used in c# ?

I used volatile because my variable is use in a Multithreaded context where this variable could be accessed/modified concurrently, but where I can loose an addition without any hurts (see code).

I added "volatile" to make sure that there is no miss alignment occurring: reading only 32 bits of the variable and the other 32 bits on another fetch which can be broken in 2 by a write in the middle from another thread.

Does my previous assumption (previous statement) can really happen of not ? If not, does "volatile" usage is still necessary (Option properties modifications could happen in any thread).

After reading the 2 first answers. I would like to insists on the fact that the way the code is written, it is not important if due to concurrency we miss an increment (want to increment from 2 threads but the result is only incremented by one due to concurrency) if at least the variable '_actualVersion' is incremented.

As reference, this is the part of code where I'm using it. It is to report save action (write to disk) only while the application is idle.

public abstract class OptionsBase : NotifyPropertyChangedBase
{
    private string _path;

    volatile private int _savedVersion = 0;
    volatile private int _actualVersion = 0;

    // ******************************************************************
    void OptionsBase_PropertyChanged(object sender, System.ComponentModel.PropertyChangedEventArgs e)
    {
        _actualVersion++;
        Application.Current.Dispatcher.BeginInvoke(new Action(InternalSave), DispatcherPriority.ApplicationIdle);
    }

    // ******************************************************************
    private void InternalSave()
    {
        if (_actualVersion != _savedVersion)
        {
            _savedVersion = _actualVersion;
            Save();
        }
    }

    // ******************************************************************
    /// <summary>
    /// Save Options
    /// </summary>
    private void Save()
    {
        using (XmlTextWriter writer = new XmlTextWriter(_path, null))
        {
            writer.Formatting = Formatting.Indented;
            XmlSerializer x = new XmlSerializer(this.GetType());

            x.Serialize(writer, this);
            writer.Close();
        }
    }

Solution 1

I've used volatile where I'm not sure it is necessary.

Let me be very clear on this point:

If you are not 100% clear on what volatile means in C# then do not use it. It is a sharp tool that is meant to be used by experts only. If you cannot describe what all the possible reorderings of memory accesses are allowed by a weak memory model architecture when two threads are reading and writing two different volatile fields then you do not know enough to use volatile safely and you will make mistakes, as you have done here, and write a program that is extremely brittle.

I was pretty sure a lock would be overkill in my situation

First off, the best solution is to simply not go there. If you don't write multithreaded code that tries to share memory then you don't have to worry about locking, which is hard to get correct.

If you must write multithreaded code that shares memory, then the best practice is to always use locks. Locks are almost never overkill. The price of an uncontended lock is on the order of ten nanoseconds. Are you really telling me that ten extra nanoseconds will make a difference to your user? If so, then you have a very, very fast program and a user with unusually high standards.

The price of a contended lock is of course arbitrarily high if the code inside the lock is expensive. Do not do expensive work inside a lock, so that the probability of contention is low.

Only when you have a demonstrated performance problem with locks that cannot be solved by removing contention should you even begin to consider a low-lock solution.

I added "volatile" to make sure that there is no misalignment occurring: reading only 32 bits of the variable and the other 32 bits on another fetch which can be broken in two by a write in the middle from another thread.

This sentence tells me that you need to stop writing multithreaded code right now. Multithreaded code, particularly low-lock code, is for experts only. You have to understand how the system actually works before you start writing multithreaded code again. Get a good book on the subject and study hard.

Your sentence is nonsensical because:

First off, integers already are only 32 bits.

Second, int accesses are guaranteed by the specification to be atomic! If you want atomicity, you've already got it.

Third, yes, it is true that volatile accesses are always atomic, but that is not because C# makes all volatile accesses into atomic accesses! Rather, C# makes it illegal to put volatile on a field unless the field is already atomic.

Fourth, the purpose of volatile is to prevent the C# compiler, jitter and CPU from making certain optimizations that would change the meaning of your program in a weak memory model. Volatile in particular does not make ++ atomic. (I work for a company that makes static analyzers; I will use your code as a test case for our "incorrect non-atomic operation on volatile field" checker. It is very helpful to me to get real-world code that is full of realistic mistakes; we want to make sure that we are actually finding the bugs that people write, so thanks for posting this.)

Looking at your actual code: volatile is, as Hans pointed out, totally inadequate to make your code correct. The best thing to do is what I said before: do not allow these methods to be called on any thread other than the main thread. That the counter logic is wrong should be the least of your worries. What makes the serialization thread safe if code on another thread is modifying the fields of the object while it is being serialized? That is the problem you should be worried about first.

Solution 2

Volatile is woefully inadequate to make this code safe. You can use low-level locking with Interlocked.Increment() and Interlocked.CompareExchange() but there's very little reason to assume that Save() is thread-safe. It sure looks like it tries to save an object that's being modified by a worker thread.

Using lock is very strongly indicated here, not just to protect the version numbers but also to prevent the object from changing while it is being serialized. The corrupted saves you'll get from not doing this are entirely too infrequent to ever have a shot a debugging the problem.

Solution 3

According to Joe Albahari's excellent post on threads and locks which is from his equally excellent book C# 5.0 In A Nutshell, he says here that even when the volatile keyword is used, that a write statement followed by a read statement can be reordered.

Further down, he says that MSDN documentation on this topic is incorrect and suggests that there is a strong case to be made for avoiding the volatile keyword altogether. He points out that even if you happen to understand the subtleties involved, will other developers down the line also understand it?

So, using a lock is not only more "correct", it is also more understandable, offers ease of adding a new feature that adds multiple update statements to the code in an atomic fashion - which neither volatile nor a fence class like MemoryBarrier can do, is very fast, and is much easier to maintain since there is much less chance of introducing a subtle bug by less experienced developers down the line.

Solution 4

Regarding to your statement whether a variable could be split in two 32 bit fetches when using volatile, this could be a possibility if you were using something bigger than Int32.

So as long as you are using Int32 you do not have an issue with what you are stating.

However, as you have read in the suggested link volatile gives you only weak guarantees and I would prefer locks so as to be on the safe side as today's machines have more than one CPU and volatile does not guarantee that another CPU won't sweep in and do something unexpected.

EDIT

Have you considered using Interlocked.Increment?

View more solutions