7

I am trying to write a multithreaded program where each thread would use a counter and then increment it.

for example:

lock(this)
{
   counter++;
   Console.WriteLine(counter); 
}

i know that for incrementation i can use:

System.Threading.Interlocked.Increment(counter);

but what about locking for both incrementing and doing something with the counter?

Thanks!

Rzassar
  • 2,117
  • 1
  • 33
  • 55
kiki
  • 323
  • 2
  • 5
  • 11
  • 2
    What specific problem are you having? – M.Babcock Mar 01 '12 at 14:29
  • If you use `lock`, you don't need Interlock.Increment. What's your question exactly? – dtb Mar 01 '12 at 14:30
  • 1
    i am referring to this article: http://www.c-sharpcorner.com/UploadFile/mahesh/InterlockingThreads11212005060555AM/InterlockingThreads.aspx – kiki Mar 01 '12 at 14:31
  • As long as 'doing something with the counter' is using it read-only there is no issue here. – H H Mar 01 '12 at 14:33
  • 1
    @HenkHolterman it depends, if 'read-only' means read-only across all threads then you are correct, however if the counter is a `long` running on a 32 bit machine you can not safely read it if other threads could update the value. That is the reason for [Interlocked.Read](http://msdn.microsoft.com/en-us/library/system.threading.interlocked.read.aspx) – Scott Chamberlain Mar 01 '12 at 14:46

5 Answers5

28

Doing this is OK:

Thread A:

var incremented_counter = Interlocked.Increment(ref counter);
Console.WriteLine(incremented_counter);

Thread B:

Interlocked.Increment(ref counter);

And doing this is OK:

Thread A:

lock (the_lock) {
   ++counter;
   Console.WriteLine(counter); 
}

Thread B:

lock (the_lock) {
   ++counter;
}

Doing this is OK but redundant:

Thread A:

lock (the_lock) {
    var incremented_counter = Interlocked.Increment(ref counter);
    Console.WriteLine(incremented_counter);
}

Thread B:

lock (the_lock) {
    Interlocked.Increment(ref counter);
}

But doing this is not OK:

Thread A:

Interlocked.Increment(ref counter);
Console.WriteLine(counter);

Thread B:

Interlocked.Increment(ref counter);

Nor is it doing this:

Thread A:

lock (the_lock) {
   ++counter;
   Console.WriteLine(counter); 
}

Thread B:

Interlocked.Increment(ref counter);

Nor is it doing this:

Thread A:

var incremented_counter = Interlocked.Increment(ref counter);
Console.WriteLine(incremented_counter);

Thread B:

lock (the_lock) {
   ++counter;
}

(BTW, don't use lock on this.)

Community
  • 1
  • 1
Branko Dimitrijevic
  • 50,809
  • 10
  • 93
  • 167
  • 1
    -1 `Interlocked.Increment` takes one reference parameter, and it reads, increments and stores it in a single atomic operation. `var incremented_counter = Interlock.Increment(ref counter);` is the correct version. – Aidiakapi Mar 21 '12 at 10:22
  • @Aidiakapi How does that make any of what I wrote incorrect? BTW, I did make a typo: it should be `Interlocked` (not `Interlock`). – Branko Dimitrijevic Mar 21 '12 at 10:30
  • Non of your examples will compile. – Aidiakapi Mar 21 '12 at 11:03
  • @Aidiakapi Ahh... I forgot `ref` (edited). But others have made the same mistake and I don't see you downvoting _them_! In any case, this is just a syntax error and not really relevant for the greater point I was trying to make - wouldn't you agree that I've correctly illustrated both the proper and the improper use of various synchronization techniques? – Branko Dimitrijevic Mar 21 '12 at 11:23
  • 4
    Yes, but the other reason I downvoted was because you only give example, say what to do and what not to do, without telling them *why* it is right or wrong. Don't get me wrong, your examples are more than correct. But things like: `var incremented_counter = Interlocked.Increment(ref counter);` might imply to somebody who has no idea what it does that counter isn't incremented and just `incremented_counter` has been incremented. But I'll upvote you :P – Aidiakapi Mar 21 '12 at 13:08
  • Why does the **first** `not ok` examples is not ok ? – Royi Namir Nov 15 '12 at 07:50
  • @RoyiNamir Because `Console.WriteLine(counter)` is accessing "naked" `counter`. The other thread might have changed the `counter` between this line and the previous line (aka. race condition), there is no memory barrier (normally provided by a lock or an interlocked operation) to make sure `counter` is not stale (due to the caching) and depending on its type and the underlying architecture `counter` might not be atomic at all, so you might be accessing "half cooked" bytes. Contrast that to the first "OK" example where `Console.WriteLine(incremented_counter)` accesses only the local variable. – Branko Dimitrijevic Nov 15 '12 at 13:31
  • @BrankoDimitrijevic oh you mean just the `Console.WriteLine(counter);` part is not ok...right ? cause the half cooked bits....right ? ( I thought there were something wrong with `Interlocked.Increment(ref counter);` itself ( in both threads) – Royi Namir Nov 15 '12 at 13:35
  • @RoyiNamir Right. There can never be anything "wrong" with either `lock` or interlocked operations in isolation - they always work exactly as they should. The difficulty of writing correct multi-threaded code arises when **combining** these primitives together or with the "useful" code. – Branko Dimitrijevic Nov 15 '12 at 13:40
  • why such difficult answer for such a simple question ? I just asked if the problem ( for it to be as `not ok`) is the console.write part... – Royi Namir Nov 15 '12 at 13:46
  • Why this is an issue? `Interlocked.Increment(ref counter); Console.WriteLine(counter);` If so, how to solve a problem where I increment the counter and somewhere else if have to compare with a kind of `if (counter > N)`? – Gianpiero Nov 05 '15 at 09:56
  • @GianpieroCaretti Because `Interlocked.Increment` in thread B may change `counter` before `Console.WriteLine` in thread A. For the other question: use `lock`. – Branko Dimitrijevic Nov 06 '15 at 09:43
5

All of the Interlock functions return a copy of the value after modification, used that returned value during your thread.

var localCounter = System.Threading.Interlock.Increment(counter);
Scott Chamberlain
  • 124,994
  • 33
  • 282
  • 431
  • 2
    `Interlocked.Exchange` and `Interlocked.CompareExchange` return the value before modification (they'd be sorta useless if they returned the 'after' value). – supercat Mar 01 '12 at 14:34
  • You are correct. I guess the lesson is [always check the MSDN](http://msdn.microsoft.com/en-us/library/55dzx06b.aspx) – Scott Chamberlain Mar 01 '12 at 14:39
  • Incidentally, I've sometimes wished for `Interlocked.PostIncrement` or `Interlocked.PostDecrement` operations, especially in vb which does not a convenient "unchecked integer add/subtract" operation. Fundamentally, they'd be the same operation as the normal `Interlocked.Increment/Decrement` (with an unchecked subtract/add following), but if one wants the value the variable had before the adjustment, specifying that in the call would seem more logical than doing the adjustment and then unadjusting the result. – supercat Mar 01 '12 at 15:54
1

You will need to protect both the reading and the writing using the lock. In that case, the lock statement works best, and is easiest to follow:

private int counter;
private readonly object locker = new object();

public void IncrementCounter()
{
    lock (this.locker)
    {
       this.counter++;
       Console.WriteLine(counter); 
    }
}

public int GetCounter()
{
    lock (this.locker)
    {
       return this.counter;
    }
}
Steven
  • 166,672
  • 24
  • 332
  • 435
0

On the question it's a Console app, if some people will try to port over the advices here to a Windows App they may catch some surprises, especially on the lock mechanism. Just to get to my point:

    object lock_obj = new object();

    private async void button1_Click(object sender, EventArgs e)
    {
        if (Monitor.TryEnter(lock_obj))
        {
            int t = await Task.Run(() => getVal());
            textBox1.Text = t.ToString(); ;
            Monitor.Exit(lock_obj);
        }
    }

    int count = 0;

    int getVal()
    {
    Thread.Sleep(1000);
    count++;
    return count;
    }

On the code above, getval will sleep for 1 second. Most people will think that Monitor.TryEnter will prevent the block of code inside the if statement being executed while getval is still in 1 sec sleep, but in fact if you press the button multiple times within 1 second, you will see the textBox1 value incrementing which may not be the intention we want based from the code. But the behavior changes if you substitute Interlocked in place of the monitor locking:

    int interlockCount=0;

    private async void button1_Click(object sender, EventArgs e)
    {
        if (Interlocked.CompareExchange(ref interlockCount, 1,0) == 0)
        {
            int t = await Task.Run(() => getVal());
            textBox1.Text = t.ToString();
            Interlocked.Exchange(ref interlockCount, 0);
        }
    }

You will see that the textBox1.Text will increment only by 1 even if you press the button a multiple times (while the getval is still in 1 second sleep). Effectively blocking the statements inside the if statement.

0

The Interlocked.Increment function both increments the counter and returns its value; the two operations are guaranteed to be atomic. Other interlocked functions exist for decrementing/reading a counter, or adding to/reading a variable. In general, one can perform almost any simple operation on an Int32 or Int64 via the pattern:

Int64 oldValue, newValue;
do
{
  oldValue = theVariable;
  newValue = oldValue | 1;  // Sample operation
} while (Interlocked.CompareExchange(theVariable, newValue, oldValue) != oldValue);

One has to be careful to get the pattern right (e.g. be certain that newValue is computed in terms of oldValue and not theVariable) but the pattern is pretty simple and versatile.

534F
  • 57
  • 6
supercat
  • 77,689
  • 9
  • 166
  • 211
  • The returning of value is not guaranteed to be atomic, only the update of the passed in variable. If another thread was reading the value of the variable that had its value set from the return of the function while it was being assigned you would get tearing. The returned value is a non thread safe local copy. – Scott Chamberlain Mar 01 '12 at 14:42
  • @ScottChamberlain: I'm 99.99999% certain that `Foo = Interlocked.Increment(Bar)` will perform an atomic sequence [temp = Bar, temp+=1, Bar=temp], followed non-atomically by the operation [Foo = temp]. The generation of the return value is guaranteed to be atomic with the increment, but there's no way the `Interlocked` function should be reasonably expected to guarantee anything about the atomicity of anything that happens *after it returns*. – supercat Mar 01 '12 at 15:50
  • Yes but your answer says "both increments the counter and returns its value; **the two operations are guaranteed to be atomic.**" to me, you where implying the assignment of Foo in `Foo = Interlocked.Increment(Bar)` would be atomic. – Scott Chamberlain Mar 01 '12 at 19:38
  • 1
    @ScottChamberlain: Sorry for any confusion. The original poster was suggesting a sequence which would be essentially "Atomic-increment variable; read variable; store it somewhere". The first two steps of that sequence can themselves be done atomically, even though the third step cannot. – supercat Mar 01 '12 at 20:44