Using volatile keyword and a lock statement

Question

I am getting the "a reference to a volatile field will not be treated as volatile" warning in an application. I understand why.

As a simple example will the below code make the issue thread safe even though I will get the warning still?

private volatile int myVal = 10;
private int myNonVolatileNumber = 50;
private static readonly object lockObject = new object();

private void ChangeValue(ref int Value)
{
  lock (lockObject)
  {
    Value = 0;
  }
}

private void MyMethod()
{
  ChangeValue(ref myVal); //Warning here
  ChangeValue(ref myNonVolatileNumber); //no warning
}

Increment was the wrong example. Please see updated question — Jon, Nov 15 '11 at 16:56
This would serialise all calls to `ChangeValue` which I doubt is what you want. — Jodrell, Nov 15 '11 at 17:03
Don't use `volatile`, ever. Read this: http://blogs.msdn.com/b/ericlippert/archive/2011/06/16/atomicity-volatility-and-immutability-are-different-part-three.aspx. Quote: "I discourage you from ever making a volatile field". — Steven, Nov 15 '11 at 17:04
@Jodrell: Sorry, can you say that again? I have never heard of that. Reference to that information please. — leppie, Nov 15 '11 at 17:05
As SLacks wrote, you may consider working with `Interlocked.CompareExchange` instead. http://msdn.microsoft.com/en-us/library/system.threading.interlocked.compareexchange.aspx — Matthias Meid, Nov 15 '11 at 17:14
@Leppie: To clarify what I mean by serialise in this context. All the functionality of `ChangeValue` would occur synchronously, one assignation of `Value` after the other, queued on access to the `lockObject`. This "serialisation" would occur regardless of the varialbes passed to `ChangeValue`. Apologies for any overloading of the term "serialise" that may have cause confusion. Have I missed the point again? — Jodrell, Nov 15 '11 at 17:35
@Jodrell: Thanks (I should have thought of that too, long day..) — leppie, Nov 15 '11 at 17:56

score 2 · Accepted Answer · answered Nov 15 '11 at 17:07

2

Locking forces memory barriers on both sides, so yes, your example is thread safe.

answered Nov 15 '11 at 17:07

dasblinkenlight

151
1

I think I'll stick with a lock! – Jon Nov 15 '11 at 17:24

score 1 · Answer 2 · answered Nov 15 '11 at 17:05

You almost answer it yourself:

ChangeValue(ref myVal); //Warning here
ChangeValue(ref myNonVolatileNumber); //no warning

There is only 1 copy of the compiled ChangeValue(), the code inside should implement the 'volatile' behaviour. But the compiler (Jitter) cannot predict all calls when it compiles. The only option would be to treat every ref parameter as volatile, that would be very inefficient.

But see @Steven's comment, volatile is as good as useless and should be avoided.

score 1 · Answer 3 · edited May 23 '17 at 12:12

1

Probably there is no need for the usage of the volatile keyword at the place you have used.

This SO Question answers where all should the volatile keyword should be used if at all:

When should the volatile keyword be used in C#?

edited May 23 '17 at 12:12

Community

1
1

answered Nov 15 '11 at 17:08

S2S2

8,322
5
37
65

score 0 · Answer 4 · answered Nov 16 '11 at 09:40

What would be wrong with

private int val = 10;
private var valLock = new object();
private int nonVolatileNumber = 50;
private var nonVolatileNumberLock = new object();

public int Value
{
    get { lock(valLock) return val; }
    set { lock(valLock) val = value; }
}

public int NonVolatileNumber
{
    get { lock(nonVolatileNumberLock) return nonVolatileNumber; }
    set { lock(nonVolatileNumberLock) nonVolatileNumber = value; }
}

, the only risk here is that subsequent code accesses the property's private member.

In the case of 32bit integers, or even 64bit integers on a 64bit system, becasue the reads wil be atomic, you could use the Interlocked class like this ...

private int val = 10;

public int Value
{
    get { return val; }
    set { Interlocked.Exchange(ref val, value); }
}

Or in the case of a more complex type you could use ReadWriterLockSlim ...

private SomeStructure complex;
private var complexLock = new ReadWriterLockSlim();

public SomeStructure Complex
{
    get
    {
        complexLock.EnterReadLock();
        try
        {
            return complex;
        }
        finally
        {
            complexLock.ExitReadlock();
        }
    }
    set
    {
        complexLock.EnterWriteLock();
        try
        {
            return complex;
        }
        finally
        {
            complexLock.ExitWritelock();
        }
    }
}

This is better than a standard lock because it allows multiple simultaneous reads.

Using volatile keyword and a lock statement

4 Answers4