Is interlocked increment followed by comparison thread safe?


Party canceled after heavy downpour

Sorry about the blog title, my imagination failed me :(.

In our internal alias someone asked the question "Is the following thread safe"

if(Interlocked.Increment(ref someInt) == CONSTANT_VAL)
{
    doSomeStuff();
}

My instant reaction was no because even though the increment is done in a thread safe way using System.Threading.Interlocked class, the comparison that follows is not safe.

My reasoning was that the "if" expression can be broken down to the following operations

  1. Fetch of someInt
  2. Increment operation
  3. Write back of someInt
  4. Comparison

The first 3 are done inside the Increment method and it provides concurrency protection and hence cannot be interleaved by another instruction.

So if two threads are running in parallel (one marked in red and the other in green) I assumed that the following interleaving is possible

  1. someInt is 4 and CONSTANT_VAL is 5
  2. Fetch of someint      -> someInt ==4
  3. Increment operation   -> someInt ==5
  4. Write back of someint -> someInt ==5
  5. Fetch of someint      -> someInt == 5
  6. Increment operation   -> someInt == 6
  7. Write back of someint -> someInt == 6
  8. comparison            -> compare 6 & CONSTANT_VAL
  9. comparison            -> compare 6 & CONSTANT_VAL

This means that the comparison of both thread will fail.

However, someone responded back that I was wrong as the return value is being used and not the written back value. This made me do some more investigation.

If I see the JITted code then it looks like

if (Interlocked.Increment(ref someInt) == CONSTANT_VAL)
00000024  lea         ecx,ds:[002B9314h] 
0000002a  call        796F1221 
0000002f  mov         esi,eax 
00000031  cmp         esi,5

The call (at 0x000002a) is to the native code inside CLR which in turn calls Win32 api (InterlockedIncrement).

However, the last 2 lines are the interesting ones. Register EAX contains the return value and comparison is happening against that and CONSTANT_VAL. So even if the second thread had already changed the value of someInt it doesn’t have any effect as the return of the first increment is being used and not the safeInt value in memory. So first comparison (step 8 above) will actually compare CONSTANT_VAL against 5 and succeed.

Comments (11)

  1. makes sense, because it is a function call, you’re not comparing against the actual variable

    notice that nowhere the documentation mentions that the incremented value is returned when calling the function

  2. Andrew Tjew says:

    The ‘safety’ is an artifact of the way the code is generated in this specific instance. I don’t think a compiler need to maintain the ‘safety’, and having to check to compiler output all the time is probably not the most productive way to do multi-threaded computing.

  3. Sandro says:

    The safety is not an artifact of the compiler, but a consequence of the fact that Interlocked.CompareExchange does an atomic compare-and-swap-return-original-value. The value is not read out of the location after the fact.

  4. Eber: The function does mention that the incremented value is returned

    "Return Value

    Type: System.Int32

    The incremented value"

    And yes this is not an artifact of the compiler. This post kind of outlines the steps I personally went through to figure out what was happening. In retrospect it was very obvious, but isn’t everything is obvious in retrospect 🙂

  5. Andriy says:

    Sorry, one question.

    As far as i understand, .the original win32 function does not return anything, it just increments the variable in the thread-safe manner.

    The .Net Interlocked.Increment, besides incrementing returns the value.

    So internaly it can act as the following

    1. Call win32 InterlockedIncrement

    2. Fetch the new value into the output parameter

    So if between line 1 and 2 another thread changes the var?

    Is it for sure  that .Net function returns the proper value?

  6. Norman Diamond says:

    MSDN’s documentation is a bit weird.  The Win32 API always returned the correct result.  The compiler intrinsic has returned the correct result starting in Windows 98 and NT4, but in older Windows versions the compiler intrinsic could return something different from the new value of the object.

    How could the compiler intrinsic depend on the Windows version instead of depending on the compiler version?  And if the compiler intrinsic simply called the Windows API, then why was the compiler intrinsic less reliable than the Windows API in Windows 95 and NT 3.5?  Really weird.

    But anyway modern versions work, so .Net works too.

  7. Andriy if you see the Win32 InterlockedIncrement API at http://msdn.microsoft.com/en-us/library/ms683614(VS.85).aspx you’d see that it does return the incremented value. I checked the BCL code as well and it does do a

    return InterlockedIncrement(Foo);

  8. Ferneu says:

    This post might enlighten you.

    http://www.eggheadcafe.com/conversation.aspx?messageid=34506793&threadid=34497502

    The base idea is: if it is simply calling the native win32 API and you manage to run .net code on a 386 or older system, the result might surprise you

  9. lixiong says:

    the register is per-thread, not thread shared, which means the thread1's eax is still 5, not 6.

  10. Tim says:

    Run this loop and explain the results please –

    Dim value As Integer

    Parallel.For(0, 1000, Sub(i)

    Debug.WriteLine(Interlocked.Increment(value).ToString)

                                                         End Sub)

    I get –

    3

    4

    5

    6

    7

    1

    2

    10

    11

    12

    13

    14

    8

    9

    17

    18

    etc etc…

  11. Tim says:

    At least it doesn't print the same value twice. So I guess the numbers are printed out of sequence because the threads are switching between the call to Interlocked.Increment and the actual output to the debug window…