I was looking for a thread-safe counter implementation using Interlocked
that supported incrementing by arbitrary values, and found this sample straight from the Interlocked.CompareExchange
documentation (slightly changed for simplicity):
private int totalValue = 0;
public int AddToTotal(int addend)
{
int initialValue, computedValue;
do
{
// How can we get away with not using a volatile read of totalValue here?
// Shouldn't we use CompareExchange(ref TotalValue, 0, 0)
// or Thread.VolatileRead
// or declare totalValue to be volatile?
initialValue = totalValue;
computedValue = initialValue + addend;
} while (initialValue != Interlocked.CompareExchange(
ref totalValue, computedValue, initialValue));
return computedValue;
}
public int Total
{
// This looks *really* dodgy too, but isn't
// the target of my question.
get { return totalValue; }
}
I get what this code is trying to do, but I'm not sure how it can get away with not using a volatile read of the shared variable when assigning to the temporary variable that is added to.
Is there a chance that initialValue
will hold a stale value throughout the loop, making the function never return? Or does the memory-barrier (?) in CompareExchange
eliminate any such possibility? Any insight would be appreciated.
EDIT: I should clarify that I understand that if CompareExchange
caused the subsequent read of totalValue
to be up to date as of the last CompareExchange
call, then this code would be fine. But is that guaranteed?
If we read a stale value, then the CompareExchange
won't perform the exchange - we're basically saying, "Only do the operation if the value really is the one we've based our calculation on." So long as at some point we get the right value, it's fine. It would be a problem if we kept reading the same stale value forever, so CompareExchange
never passed the check, but I strongly suspect that the CompareExchange
memory barriers mean that at least after the time through the loop, we'll read an up-to-date value. The worst that could happen would be cycling forever though - the important point is that we can't possibly update the variable in an incorrect way.
(And yes, I think you're right that the Total
property is dodgy.)
EDIT: To put it another way:
CompareExchange(ref totalValue, computedValue, initialValue)
means: "If the current state really was initialValue
, then my calculations are valid and you should set it to computedValue
."
The current state could be wrong for at least two reasons:
initialValue = totalValue;
assignment used a stale read with a different old valuetotalValue
after that assignmentWe don't need to handle those situations differently at all - so it's fine to do a "cheap" read so long as at some point we'll starting seeing up-to-date values... and I believe the memory barriers involved in CompareExchange
will ensure that as we loop round, the stale value we see is only ever as stale as the previous CompareExchange
call.
EDIT: To clarify, I think the sample is correct if and only if CompareExchange
constitutes a memory barrier with respect to totalValue
. If it doesn't - if we can still read arbitrarily-old values of totalValue
when we keep going round the loop - then the code is indeed broken, and may never terminate.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With