views:

138

answers:

1

Is this code thread-safe? or put it this way:

Is there anyway to call GetIt() and that GetIt() will return the same number to 2 different threads

Private Shared hitCount As Long = 1

Public Shared Function GetIt() As Long
     Threading.Interlocked.Increment(hitCount)
     DoSomethingQuick(hitCount)
     Return hitCount
End Function

It seems like it's possible, then am I supposed to use Interlocked.Read() or lock the whole thing in one block?

+9  A: 

Yes there is a possibility:

  1. Thread 1 runs Threading.Interlocked.Increment(hitCount)
  2. Thread 2 runs Threading.Interlocked.Increment(hitCount)
  3. Thread 1 runs Return hitCount
  4. Thread 2 runs Return hitCount

In steps 3 and 4, hitCount will be the same value.

But the fix is easy Interlocked.Increment returns the incremented value, so just change your code to:

Private Shared hitCount As Long = 1L

Public Shared Function GetIt() As Long
     Return Threading.Interlocked.Increment(hitCount)
End Function

Edit Or now based on your edit, you have a pretty bit timing hole. Anyway then this is what you want:

Public Shared Function GetIt() As Long
     Dim localHitCount As Long = Threading.Interlocked.Increment(hitCount)
     Console.Writeline("Something, something....")
     Return localHitCount 
End Function

Edit Then do this (which is exactly what Michael suggested below)

Private Shared hitCount As Long = 1L

Public Shared Function GetIt() As Long
     Dim localHitCount As Long = Threading.Interlocked.Increment(hitCount)
     DoSomethingQuick(localHitCount )
     Return localHitCount 
End Function
shf301
:) I didn't know that it was returning the final value, nice one.
dr. evil
Just updated the code to make same with my original code (sorry I badly wrote the sample code). AFAIK in this updated code I have to wrap the whole thing with a lock.
dr. evil
You could just cache the return value from Interlocked.Increment and use that in the call to DoSomethingQuick and the return.
Michael
Thanks Michael! Darn! Why didn't I think that? :) That was obvious wasn't it :D
dr. evil
You don't need but the whole thing is a lock, unless you are relying on hitCount not changing at all during the run, rather than just ensuring that you have a unique value.
shf301
@shf301: Firstly, `localHitCount` should be a `Long`, not an `int`; otherwise this method could throw an overflow exception when `hitCount` exceeds `Integer.MaxValue`. Secondly, you seem to have mixed C# and VB in your code...
Dan Tao
@shf301: Actually, I just realized I could edit it for you :)
Dan Tao