4

In the code below, if the GetNextNumber() is called by two threads concurrently is possible that it returns same number to both the threads?

class Counter
{
 private static int s_Number = 0;
 public static int GetNextNumber()
 {
  s_Number++;
  return s_Number;
 }
}

Can you explain why?

EDIT: If it is possible for the code to return same number to both threads then is the following correct? Let's say the two threads call GetNextNumber() when s_Number is equal to 2. If the same value is returned then that value can only by 4. It cannot be 3. Is that correct?

8
  • 8
    Yes it is possible Commented Apr 12, 2013 at 8:50
  • It's possible, though not guaranteed Commented Apr 12, 2013 at 8:50
  • 5
    It's unlikely, but because it's possible you should handle the situation. I would use the lock keyword. Commented Apr 12, 2013 at 8:51
  • 1
    Come on guys! If you say "it is possible.." please suggest informations to him which provide your opinions.. Commented Apr 12, 2013 at 8:53
  • 1
    Not so, look at the IL code generated for your class. Commented Apr 12, 2013 at 9:00

4 Answers 4

11

When dealing with such a simple counter, it's best to use Interlocked.Increment:

private static int s_Number = 0;

public static int GetNextNumber()
{
    return Interlocked.Increment(ref s_Number);
}

This will ensure each thread will return a unique value (as long as the number doesn't overflow), and that no increments will be lost.

Since the original code can be broken down into these steps:

  1. Read the existing value of s_Number
  2. Add 1 to it
  3. Store the new value into s_Number
  4. Read s_Number
  5. Return the read value

The scenarios that can occur are:

  1. Both threads execute step 1 before the rest, which means both threads will read the same existing value, increment by 1, ending up on the same value. Lost an increment
  2. The threads can execute step 1 through 3 without conflict, but end up executing step 4 after both threads have updated the variable, retrieving the same value. Skipped a number

For larger pieces of code, where more data needs to be accessed atomically, a lock statement is usually a better way:

private readonly object _SomeLock = new object();

...

lock (_SomeLock)
{
    // only 1 thread allowed in here at any one time
    // manipulate the data structures here
}

But for such a simple piece of code, where all you need to do is atomically increment a field and retrieve the new value, Interlocked.Increment is better, faster, less code.

There are other methods on the Interlocked class as well, they're really handy in the scenarios they handle.

More detailed explanation of lost an increment.

Let's assume that s_Number starts at 0 before the two threads execute:

Thread 1                            Thread 2
Read s_Number = 0
                                    Read s_Number = 0
Add 1 to s_Number, getting 1
                                    Add 1 to s_Number, getting 1 (same as thread 1)
Store into s_Number (now 1)
                                    Store into s_Number (now 1)
Read s_Number = 1
                                    Read s_Number = 1
Return read value (1)
                                    Return read value (1)

As you can see above, the final value of s_Number should've been 2, and one of the threads should've returned 1, the other 2. Instead the final value was 1, and both threads returned 1. You lost an increment here.

Detailed explanation of skipping a number

Thread 1                            Thread 2
Read s_Number = 0
Add 1 to s_Number, getting 1
Store into s_Number (now 1)
                                    Read s_Number = 1
                                    Add 1 to s_Number, getting 2
                                    Store into s_Number (now 2)
Read s_Number = 2
                                    Read s_Number = 2
Return read value (2)
                                    Return read value (2)

Here the final result of s_Number will be 2, which is correct, but one of the threads should've returned 1, instead they both returned 2.

Let's see how the original code looks on an IL level. I'll add the original code into the IL instructions with comments

// public static int GetNumber()
// {
GetNumber:
//     s_Number++;
IL_0000:  ldsfld      UserQuery.s_Number    // step 1: Read s_Number
IL_0005:  ldc.i4.1                          // step 2: Add 1 to it
IL_0006:  add                               //         (part of step 2)
IL_0007:  stsfld      UserQuery.s_Number    // step 3: Store into s_Number
// return s_Number;
IL_000C:  ldsfld      UserQuery.s_Number    // step 4: Read s_Number
IL_0011:  ret                               // step 5: Return the read value
// }

Note, I used LINQPad to get the IL code above, enabling optimizations (small /o+ in the lower right corner), if you want to play with the code to see how it converts into IL, download LINQPad and feed it this program:

void Main() { } // Necessary for LINQPad/Compiler to be happy

private static int s_Number = 0;
public static int GetNumber()
{
    s_Number++;
    return s_Number;
}
Sign up to request clarification or add additional context in comments.

3 Comments

@Lasse +1 for the scenarios. Can you explain the Lost scenario? The line I'm taking is: since s_Number is a value on the stack, as long as it's incremented twice, it cannot have lost increment.
okay, got it. I forgot that every thread gets its own separate stack and the value type is copied.
I outlined everything in more detail, please refresh
8

Yes, here is a scenario :

s_number = 0

Thread A do s_number ++

s_number = 1

Thread B do s_number ++

s_number = 2

Thread A do return s_number

Thread B do return s_number

Both threads returned 2.


Therefore, you should implement a locking mechanism like this :

class Counter
{
    private static int s_Number = 0;
    private static object _locker = new object();
    public static int GetNextNumber()
    {
        //Critical section
        return Interlocked.Increment(ref s_Number);
    }
}

The locking mechanism will prevent multiple threads to enter your critical section at the same time. If you have more operation than a simple increment, use a Lock block instead.

Edit: A more in-depth answer has been written by Lasse V. Karlsen, explaining more low-level behaviours.

2 Comments

+1, although in this situation I'd probably use Interlocked.Increment rather than a full lock.
@Luke Marlin, thanks. However I know how to fix the problem. What I'm interested in is the reason why.
0

return Interlocked.Increment(ref s_Number);

This will do it. It's much simpler than using lock. Locking blocks should mainly be used for blocks of code, usually.

Comments

0

It is easy to see why it is possible to get the same number when two threads try to access the concurrently the GetNextNumber method if we look at IL code generated for your class method

class Counter
{
    private static int s_Number = 0;
    public static int GetNextNumber()
    {
        s_Number++;
        return s_Number;
    }
}

Here below the IL Code generated, and, as you can see, the s_number++ is really composed by three separate instructions that could be accessed concurrently by two thread and thus getting the same initial value.

Counter.GetNextNumber:
IL_0000:  ldsfld      UserQuery+Counter.s_Number
IL_0005:  ldc.i4.1    
IL_0006:  add         
IL_0007:  stsfld      UserQuery+Counter.s_Number
IL_000C:  ldsfld      UserQuery+Counter.s_Number
IL_0011:  ret         

This is the SCENARIO that leads to the same value for both threads

The thread A enters and get the value of s_Number (IL_0000), it loads the value 1 but, at this point, the processor suspend the thread A and starts the thread B. Of course the value stored in the memory location defined for s_number is still 0 and the thread B start with the same value used byt thread A. It gets back 1. When thread A resumes its registers are restored as they were at the suspension time, so it adds 1 to 0 and gets back the same result of thread B.

This class use the lock keyword to block the concurrency

class CounterLocked
{
 private static object o;
 private static int s_Number = 0;
 public static int GetNextNumber()
 {
  lock(o)
  {
    s_Number++;
    return s_Number;
  }
 }
}

CounterLocked.GetNextNumber:
IL_0000:  ldc.i4.0    
IL_0001:  stloc.0     // <>s__LockTaken0
IL_0002:  ldsfld      UserQuery+CounterLocked.o
IL_0007:  dup         
IL_0008:  stloc.2     // CS$2$0001
IL_0009:  ldloca.s    00 // <>s__LockTaken0
IL_000B:  call        System.Threading.Monitor.Enter
IL_0010:  ldsfld      UserQuery+CounterLocked.s_Number
IL_0015:  ldc.i4.1    
IL_0016:  add         
IL_0017:  stsfld      UserQuery+CounterLocked.s_Number
IL_001C:  ldsfld      UserQuery+CounterLocked.s_Number
IL_0021:  stloc.1     // CS$1$0000
IL_0022:  leave.s     IL_002E
IL_0024:  ldloc.0     // <>s__LockTaken0
IL_0025:  brfalse.s   IL_002D
IL_0027:  ldloc.2     // CS$2$0001
IL_0028:  call        System.Threading.Monitor.Exit
IL_002D:  endfinally  
IL_002E:  ldloc.1     // CS$1$0000
IL_002F:  ret        

The code generated for the InterlockIncrement is very simple

public static int GetNextNumber()
{
    return Interlocked.Increment(ref s_Number);
}

CounterLocked.GetNextNumber:
IL_0000:  ldsflda     UserQuery+CounterLocked.s_Number
IL_0005:  call        System.Threading.Interlocked.Increment
IL_000A:  ret  

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.