tags:

views:

883

answers:

6
A: 

This (atomicity) is NOT guaranteed.

Yossarian
+5  A: 

Using the volatile keyword does not make access thread-safe, it just makes sure that reads of the variable are read from memory instead of possibly being read from a register where it is cached from a previous read. Certain architectures make this optimisation which can lead to stale values being used where you have multiple threads writing to the same variable.

In order to synchronize access properly, you'll need to have a wider lock:

public class Xyz
{
    private volatile int _lastValue;
    private IList<int> AvailableValues { get; set; }
    private object syncRoot = new object();
    private Random rand = new Random();

    //Accessible by multiple threads
    public int GetNextValue() //and return last value once store is exhausted
    {
        //...

        lock (syncRoot)
        {
            var count = AvailableValues.Count;
            if(count == 0)
                return _lastValue;

            toReturn = rand.Next(0, count);
            _lastValue = AvailableValues[toReturn];
            AvailableValues.RemoveAt(toReturn);
        }
        return _lastValue;
    }
}

If performance is a concern you might want to consider using a LinkedList for AvailableValues, since it supports an O(1) remove operation.

Matt Howells
@Matt: Thanks... I wanted to hold the lock for as less a duration as possible. This actually synchronizes the whole method. Only the collection being used need to be accessed synchronously.
Vyas Bharghava
@Matt: I've moved Random() out... thanks for the bug fix. :)
Vyas Bharghava
I've edited to minimise the locking.
Matt Howells
+10  A: 

volatile is actually more related to caching (in registers etc); with volatile you know that that value is actually written-to/read-from memory immediately (which isn't actually always the case otherwise). This allows different threads to immediately see updates from each other. There are other subtle issues with instruction re-ordering, but that gets complex.

There are two meanings of "atomic" to consider here:

  • is a single read atomic by itself / write atomic by itself (i.e. could another thread get two different halves of two Doubles, yielding a number that never actually existed)
  • is a read/write pair atomic/isolated together

The "by itself" depends on the size of the value; can it be updated in a single operation? The read/write pair is more to do with isolation - i.e. preventing lost updates.

In your example, it is possible for two threads to read the same _lastValue, both do the calculations, and then (separately) update _lastValue. One of those updates is going to get lost. In reality, I expect you want a lock over the duration of the read/write process.

Marc Gravell
+1 My understanding was that there were some quirks with the way IA64 reorder instructions, that never happen with volatile on x64 x32
Sam Saffron
Quite possibly - like I said "that gets complex" ;-p Fortunately I don't see a lot of IA64...
Marc Gravell
@Marc: Yes, I meant atomic as in your def #1. Value gets lost is not an issue (in this particular case) as I want to hold only the last value... In the revised code we have only read operation once the store is exhaused.
Vyas Bharghava
`int` is guaranteed to be atomic under the meaning of #1
Marc Gravell
A: 

They are for some types link. In you case it is a int so it is atomic according to C# specs. But as others in this topic, it does not guarantee you code is thread-safe.

Cedrik
+2  A: 

For .Net 2.0 and before, there is a class called ReaderWriterLock which allows you to block writes and reads separately. Might be helpful.

For .Net 3.5 and above, consider ReaderWriterLockSlim, which microsoft describe like this;

ReaderWriterLockSlim is similar to ReaderWriterLock, but it has simplified rules for recursion and for upgrading and downgrading lock state. ReaderWriterLockSlim avoids many cases of potential deadlock. In addition, the performance of ReaderWriterLockSlim is significantly better than ReaderWriterLock. ReaderWriterLockSlim is recommended for all new development.

Steve Cooper
`ReaderWriterLockSlim` is usually recommended over `ReaderWriterLock`: http://msdn.microsoft.com/en-us/library/system.threading.readerwriterlockslim.aspx
LukeH
Excellent! thanks for that. It's new to .net 3.5, and in my day-to-day we're still on .net 2.0.
Steve Cooper
Good point. I forgot that it was only introduced in the 3.5 framework.
LukeH
+9  A: 

My understanding of C# says (thanks to Jeff Richter & Jon Skeet) that assignment is "atomic".

Assignment is not atomic in general. The C# specification carefully calls out what is guaranteed to be atomic. See section 5.5:

Reads and writes of the following data types are atomic: bool, char, byte, sbyte, short, ushort, uint, int, float, and reference types. In addition, reads and writes of enum types with an underlying type in the previous list are also 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.

(Emphasis added.)

If have only Read & assign would both the operations be atomic?

Again, section 5.5 answers your question:

there is no guarantee of atomic read-modify-write

Eric Lippert
@Eric: Thanks... That helped.
Vyas Bharghava