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.

Emelda

Have you considered using a ReaderWriterLock(Slim)? Are you sure that a simple lock does not satisfy your needs? Why not? What is the usage pattern for the SetOnce class? – Tracee

I had forgotten about ReaderWriterLock, I'll give it a lock. But just like the regular lock, I think it will introduce a synchronization overhead at each read, which I'd rather avoid since the value is only changed once, so when reading a non-null value (without locks), one has the guarantee that it is not stale. – Emelda

ReaderWriterLock(Slim) is indeed better then ordinal lock, since it allows multiple simultaneous reads. – Vitalism

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();
    }
}

Ezekiel

The main problem I see is that in GetValue you can get a stale value (NoValueSetError) after having called SetValue in another thread. Apart from that, your solution is quite funny and imaginative :) I enjoyed reading it ! – Emelda

@GeorgesDupéron Yeah, that's what I mean about volatility. Maybe that can be resolved by marking the ValueGetter field as volatile? Honestly, I haven't had the need to really work with volatile fields (I stick to lock blocks mainly) I figured if you happen to hit GetValue while SetValue is executing, then what else are you to do anyway at that point? EDIT: If someone can confirm that marking the field as volatile or some other tweak avoids that possible "stale" issue, then I'll happily edit the answer. – Ezekiel

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;
    }
}

Lvov

Notice, using this as lock target is a bad idea (lock(this)), it's safer to declare separate private object field for this purpose – Vitalism

No, another cannot set _value to null: My case is special in that _value is written only once after its initialization. It is initialized to null, then at some point in time it will be set to a non-null value, then is never modified. – Emelda