Read an up-to date value from an Interlocked variable, with only one write on the variable

Question

I would like to create a class with two methods:

• void SetValue(T value) stores a value, but only allows storing a single value (otherwise it throws an exception).
• T GetValue() retrieves the value (and throws an exception if there is no value yet).

I have the following desires/constraints:

• Reading the value should be cheap.
• Writing the value can be (moderately) costly.
• GetValue() should throw the exception only if the up-to-date value is absent (null): It should not throw an exception based on a stale null value after a call to SetValue() in another thread.
• The value is written only once. This means GetValue() does not need to freshen the value if it is not null.
• If a full memory barrier can be avoided, then it is (much) better.
• I get that lock-free concurrency is better, but I'm not sure if it is the case here.

I came up with several ways to achieve that, but I'm not sure which are correct, which are efficient, why they are (in)correct and (in)efficient, and if there is a better way to achieve what I want.

Method 1

• Using a non-volatile field
• Using Interlocked.CompareExchange to write to the field
• Using Interlocked.CompareExchange to read from the field
• This relies on the (possibly wrong) assumption that after doing an Interlocked.CompareExchange(ref v, null, null) on a field will result in the next accesses getting values that are at least as recent as those that Interlocked.CompareExchange saw.

The code:

public class SetOnce1<T> where T : class
{
    private T _value = null;

    public T GetValue() {
        if (_value == null) {
            // Maybe we got a stale value (from the cache or compiler optimization).
            // Read an up-to-date value of that variable
            Interlocked.CompareExchange<T>(ref _value, null, null);
            // _value contains up-to-date data, because of the Interlocked.CompareExchange call above.
            if (_value == null) {
                throw new System.Exception("Value not yet present.");
            }
        }

        // _value contains up-to-date data here too.
        return _value;
    }

    public T SetValue(T newValue) {
        if (newValue == null) {
            throw new System.ArgumentNullException();
        }

        if (Interlocked.CompareExchange<T>(ref _value, newValue, null) != null) {
            throw new System.Exception("Value already present.");
        }

        return newValue;
    }
}

Method 2

• Using a volatile field
• Using Ìnterlocked.CompareExchangeto write the value (with [Joe Duffy](http://www.bluebytesoftware.com/blog/PermaLink,guid,c36d1633-50ab-4462-993e-f1902f8938cc.aspx)'s#pragmato avoid the compiler warning on passing a volatile value byref`).
• Reading the value directly, since the field is volatile

The code:

public class SetOnce2<T> where T : class
{
    private volatile T _value = null;

    public T GetValue() {
        if (_value == null) {
            throw new System.Exception("Value not yet present.");
        }
        return _value;
    }

    public T SetValue(T newValue) {
        if (newValue == null) {
            throw new System.ArgumentNullException();
        }

        #pragma warning disable 0420
        T oldValue = Interlocked.CompareExchange<T>(ref _value, newValue, null);
        #pragma warning restore 0420

        if (oldValue != null) {
            throw new System.Exception("Value already present.");
        }
        return newValue;
    }
}

Method 3

• Using a non-volatile field.
• Using a lock when writing.
• Using a lock when reading if we read null (we will get a coherent value outside the lock since references are read atomically).

The code:

public class SetOnce3<T> where T : class
{
    private T _value = null;

    public T GetValue() {
        if (_value == null) {
            // Maybe we got a stale value (from the cache or compiler optimization).
            lock (this) {
                // Read an up-to-date value of that variable
                if (_value == null) {
                    throw new System.Exception("Value not yet present.");
                }
                return _value;
            }
        }
        return _value;
    }

    public T SetValue(T newValue) {
        lock (this) {
            if (newValue == null) {
                throw new System.ArgumentNullException();
            }

            if (_value != null) {
                throw new System.Exception("Value already present.");
            }

            _value = newValue;

            return newValue;
        }
    }
}

Method 4

• Using a volatile field
• Writing the value using a lock.
• Reading the value directly, since the field is volatile (we will get a coherent value even if we don't use a lock since references are read atomically).

The code:

public class SetOnce4<T> where T : class
{
    private volatile T _value = null;

    public T GetValue() {
        if (_value == null) {
            throw new System.Exception("Value not yet present.");
        }
        return _value;
    }

    public T SetValue(T newValue) {
        lock (this) {
            if (newValue == null) {
                throw new System.ArgumentNullException();
            }

            if (_value != null) {
                throw new System.Exception("Value already present.");
            }

            _value = newValue;

            return newValue;
        }
    }
}

Other methods

I could also use Thread.VolatileRead() to read the value, in combination with any of the writing techniques.

Answer 1

Well, not sure about volatility, but if you don't mind a little abuse and invoking a second method... (also it's not dependent on nullability; freely usable for value types) Avoids null checks in the getter too. Only locking is done on the write, so AFAIK, the only negative impact comes from invoking the delegate when getting the value.

public class SetOnce<T>
{
    private static readonly Func<T> NoValueSetError = () => { throw new Exception("Value not yet present.");};

    private Func<T> ValueGetter = NoValueSetError;
    private readonly object SetterLock = new object();

    public T SetValue(T newValue)
    {
        lock (SetterLock)
        {
            if (ValueGetter != NoValueSetError)
                throw new Exception("Value already present.");
            else
                ValueGetter = () => newValue;
        }

        return newValue;
    }

    public T GetValue()
    {
        return ValueGetter();
    }
}

In fact, I feel really silly about this and feels a little abusive. I'd be curious to see comments about potential issues of doing this. :)

EDIT: Just realized that this means the first call to SetValue(null) means that "null" will be considered a valid value and will return null without exception. Not sure if this is what you would want (I don't see why null couldn't be a valid value, but if you want to avoid it just do the check in the setter; no need in the getter)

EDITx2: If you still want to constrain it to class and avoid null values, a simple change might be:

public class SetOnce<T> where T : class
{
    private static readonly Func<T> NoValueSetError = () => { throw new Exception("Value not yet present.");};

    private Func<T> ValueGetter = NoValueSetError;
    private readonly object SetterLock = new object();

    public T SetValue(T newValue)
    {
        if (newValue == null)
            throw new ArgumentNullException("newValue");

        lock (SetterLock)
        {
            if (ValueGetter != NoValueSetError)
                throw new Exception("Value already present.");
            else
                ValueGetter = () => newValue;
        }

        return newValue;
    }

    public T GetValue()
    {
        return ValueGetter();
    }
}

Answer 2

None of your methods except the one with lock (#3) will work properly.

Look:

    if (_value == null) {
        throw new System.Exception("Value not yet present.");
    }
    return _value;

this code is not atomic and not thread-safe, if not inside lock. It is still possible that other thread sets _value to null between if and return. What you could do is to set to local variable:

    var localValue = _value;
    if (localValue == null) {
        throw new System.Exception("Value not yet present.");
    }
    return localValue;

But still it could return stall value. You would better use lock - clear, easy and fast.

Edit: Avoid using lock(this), because this is visible outside and third-party code may decide to lock on your object.

Edit 2: if null value can never be set then just do:

public T GetValue() {
    if (_value == null) {
        throw new System.Exception("Value not yet present.");
    }
    return _value;
}

public T SetValue(T newValue) {
    lock (writeLock)
    {        
        if (newValue == null) {
            throw new System.ArgumentNullException();
        }
        _value = newValue;
        return newValue;
    }
}