2

This is my code:

    Thread _th1, _th2, _th3, _th4;
int _i;

    void thMethod()
    {
        while(_i < 100){
            Thread.Sleep((new Random()).Next(1,500));
            Application.DoEvents();
            Console.WriteLine(_i);
            _i++;
        }
    }

    private void button4_Click(object sender, EventArgs e)
    {
        _th1 = new Thread(new ThreadStart(thMethod));
        _th1.Start();
        _th2 = new Thread(new ThreadStart(thMethod));
        _th2.Start();
        _th3 = new Thread(new ThreadStart(thMethod));
        _th3.Start();
        _th4 = new Thread(new ThreadStart(thMethod));
        _th4.Start();
    }

What i want to do is, doing Console.WriteLine from 0 to 99 using multithread, and the delay is random.

However, my code prints duplicated numbers

well, this is the result.

0 1 2 2 4 5 6 7 8 9 10 10 12 13 14 14 16 16 18 19 20 20 22 22 24 24 26 26 28 28 30 31 32 33 34 35 36 36 38 39 40 40 42 42 44 44 46 46 48 49 50 50 52 52 54 54 56 56 58 58 60 60 62 62 64 65 66 66 68 68 70 70 72 72 74 74 76 76 78 78 80 80 82 82 84 84 86 87 88 88 90 90 92 92 94 94 96 96 98 98 100 101 102

As you see, there are duplicated numbers printed, and it does not even stop at 99.

So... How to correct this code in order to make it run properly?

====================== Do i have to change like..

    void thMethod()
    {
        while(_i < 100){
            Thread.Sleep((new Random()).Next(1,500));
            Application.DoEvents();
            Interlocked.Increment(ref _i);
            Console.WriteLine(_i);
        }
    }

this?

mik4n
  • 63
  • 7

6 Answers6

3

Although you should be using Interlocked.Increment as mentioned by other posts. This isn't the exact issue you're having.

The reason your numbers go past 100 is because the line you are incrementing the value comes after the Sleep. This causes threads to enter the loop, but by the time they write out the value the value has already changed to be above 100.

For example t1, t2, and t3 enter the loop and wait for 500ms and i is 99. Then t4 exits and increments it to 100, now t1 will print 100, t2 will print 101, t3 will print 102.

Interlocked.Increment wont actually solve the issue because the Thread.Sleep will still cause inconsistencies when the value is actually written out.

To solve the problem you would need to use a thread lock declare a variable to lock on as you can't lock on _i as its not a reference type:

object obj = new object();

void thMethod()
{
    while (_i < 100)
    {
        lock (obj)
        {
            Console.WriteLine(_i);
            _i++;
        }
        Thread.Sleep((new Random()).Next(1, 500));
    }
}

This way you will get no duplicates and the value will stop at 99.

Conrad Frix
  • 51,984
  • 12
  • 96
  • 155
David Esteves
  • 1,614
  • 2
  • 15
  • 22
1

You need to call Interlocked.Increment to increment i in a thread-safe fashion.

SLaks
  • 868,454
  • 176
  • 1,908
  • 1,964
1

In a multi-thread program, a shared resource with multiple writers needs some kind of a synchronization mechanism.

What happens is several threads are accessing the variable for printing before one of them has the chance to increment it, aka race condition.

To solve this you need to increment the variable in a critical section:

    void thMethod()
    {
        while (_i < 100)
        {
            Thread.Sleep((new Random()).Next(1, 500));
            Application.DoEvents();
            lock(this)
            {
                Console.WriteLine(_i);
                _i++;
            }
        }
    }

Now this won't stop at 100, so another fix will be:

        while (_i < 100)
        {
            Thread.Sleep((new Random()).Next(1, 500));
            Application.DoEvents();
            lock(this)
            {
                if (_i >= 100) break;
                Console.WriteLine(_i);
                _i++;
            }
        }
Amirshk
  • 8,170
  • 2
  • 35
  • 64
  • I replaced '_i++' -> 'Interlocked.Increment(ref _i) and this results the same... Sorry, but is there any examples?.. – mik4n Jul 30 '12 at 22:06
  • checked to make sure, and you are right. Interlocked.Increment won't help because the problem is the combination of print and increment. You need to lock the entire section. Will fix my answer. – Amirshk Jul 30 '12 at 22:14
  • 1
    From the [lock Statement (C# Reference) msdn doc](http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx) *In general, avoid locking on a public type, or instances beyond your code's control. The common constructs **lock (this)**, lock (typeof (MyType)), and lock ("myLock") violate this guideline.* Also see [Why is lock(this) {…} bad?](http://stackoverflow.com/q/251391/119477) – Conrad Frix Jul 30 '12 at 22:26
0

The variable _i must be declared in the thMethod, not at a global level.

bittenbytailfly
  • 233
  • 1
  • 7
0

There are two options other then using Monitor that weren't explored

If you're just incrementing an integral type using one of the Interlocked methods offers the best performance. Since you're also using this variable to signal completion Interlocked.CompareExchange is a natural fit. This technique is used in lock-free and wait-free solutions.

while (_i < 100)
{
    Thread.Sleep((new Random()).Next(1, 500));
    int initI;
    int modifiedI;
    bool valueUpdated = false;
    do
    {
        initI = _i;
        modifiedI = initI + 1;

        if (modifiedI > 100)
            break;

        valueUpdated =  (initI == Interlocked.CompareExchange(
                ref _i, modifiedI, initI) );

        if (valueUpdated)
            Console.WriteLine(modifiedI);

    } while (!valueUpdated)  ;


}

If you're going for simplicity you can also use the Parallel Class

Parallel.For(0, 100, i =>
    {
        Thread.Sleep((new Random()).Next(1, 500));
        _i++;
        Console.WriteLine("{0} | {1}" ,Thread.CurrentThread.ManagedThreadId, _i );
    }
);

For demonstration purposes I added the output of the ManagedThreadId so you can see that more than one thread was in use.

You should note that you lose direct control of the number of threads created instead you can pass in ParallelOption with the MaxDegreeOfParallelism set which does influence the number of threads .

Conrad Frix
  • 51,984
  • 12
  • 96
  • 155
-1

I solved the problem with interlocked. This is a vb.net console app, but is easily understandable. Every thread gets its unique counter, obviously the output sequence is not ordered


Module Module1

Dim count As Integer = 0

Sub Main()
    For i = 1 To 10
        Dim th As New Thread(AddressOf ThreadedIncrement)
        th.Start()
    Next
    Console.ReadLine()
End Sub

Sub ThreadedIncrement()
    Dim r As New Random

    For i = 1 To 10
        Thread.SpinWait(r.Next(1000, 1000000))
        Dim ncount As Integer = Interlocked.Add(count, 1)
        Console.WriteLine(ncount.ToString)
    Next
End Sub

End module


Platypus
  • 61
  • 6