What's the point in Max(Threading.Interlocked.Increment(Offset), Offset - 1)?

Question

I have seen the mentioned piece of code on several occasions now, what's the point of doing a Max(a+1, a-1)? At first I thought it might be to prevent underflow, but it doesn't really make sense not to prevent the underflow in that case.

Answer 1

Well, it's not the same as Max(a, a-1) - because there's the call to Interlocked.Increment. Suppose another thread changes the value of Offset between the call to Interlocked.Increment and the evaluation of Offset-1.

However, it still sounds odd to me - because unless Offset is volatile, the latter expression may not get the latest value anyway.

Having said all this, the code can't quite be

Math.Max(Interlocked.Increment(Offset), Offset-1)

because the parameter to Interlocked.Increment is a ref parameter. The closest legal code would be:

Math.Max(Interlocked.Increment(ref Offset), Offset-1)

(This would still be odd as it means a Pascal-cased variable...)

Perhaps if you could show the exact code in context we could work out the point. I can't say I've ever seen such code myself.

Answer 2

Interlocked.Increment(a) increments the value of a atomically. In a single-threaded application it is equivalent to a++, but is somewhat slower. In a multithreaded application it ensures that when both threads increment the value, they do so without interfering with one another. See the MSDN reference page for more details

Why it is being used in this context I cannot say without seeing more code.

Answer 3

As you mentioned, it's possible they are trying to detect when Offset wraps to zero (or goes negative) and keep it pegged at the max value for whatever type Offset is. Or they may be trying to deal with a situation where Offset is 'simultaneously' incremented in multiple threads (or something).

In either case, this is buggy code.

If they are trying to detect/prevent wrap to zero or negative, one too many increments will cause the problem anyway. If they are trying to deal with Offset being incremented 'simultaneously', I'm not sure what problem they're really trying to solve or how they are trying to solve it.

Here are the problems:

As Jon Skeet says:

However, it still sounds odd to me - because unless Offset is volatile, the latter expression may not get the latest value anyway.

But it's even worse than that - even if Offset is volatile there's nothing to serialize reading the new value of Offset with another thread that may be incrementing it, so the very instant after the Max() expression re-reads the value of Offset any number of other threads may come along and increment it (any number of times). So at best, the expression is useless, but using that technique can be harmful because you may end up using a value of Offset that doesn't 'belong' to you.

Consider the situation where you're using Offset to track an array index or something (which given the name sounds like exactly what might be going on). When you atomically increment Offset the array element at that index becomes yours to use. If you use the Max() expression as in the question, you may suddenly be trampling on an array element that really belongs to another thread.

I can't think of a legitimate reason to use

Max(Threading.Interlocked.Increment(Offset), Offset - 1)

Like I said, at best it's harmless (and useless), but it could introduce very hard to diagnose problems - don't use it.

---

Thanks to the comment by Simon Svensson that pointed to a Google search for the usage in the question, it looks like this code usually (always?) comes from the output of a C# to VB.NET converter. It even uses this expression to increment local variables (so threading isn't an issue). In the cases where Offset is a local variable, this expression falls into the useless but mostly harmless variety (there will be a 'bug' if the increment wraps). It's really more or less a very inefficient Offset++ expression (in fact it appears to be the way the converter converts the C# expression Offset++ to VB.NET - see http://www.telerik.com/community/forums/open-source-projects/code-converter/added-value.aspx#307755). Actually this conversion is buggy for Offset++ anyway because it returns the incremented value when it should return what the value was before incrementing.

The really bad thing is that other people may look at that code and think, "so that's the way I need to use Interlocked.Increment()".

Answer 4

A bit of googling gives me a suspicion that this might arise from some (possibly buggy) C# to VB.NET convertor software. That would explain the frequent appearance of it.

Added: Yessss, I found it! Try entering the following C# code in it and convert to VB.NET:

int i;
i++;