Why there is thread race at the beginning?

Question

I recently tried to write an example for lock statement. Consider following code:

public partial class Form1 : Form
    {
        private class Concurrency
        {
            private int _myValue;
            private object _locker = new object();
            public int Value
            {
                set
                {
                    lock (_locker)
                    {
                        _myValue = value;
                        Thread.Sleep(new Random().Next(5, 25));
                    }
                }

                get
                {
                    return _myValue;
                }
            }
        }

        private Random _random;
        private Concurrency _concurrency;

        public Form1()
        {
            InitializeComponent();
            _random = new Random();
            _concurrency = new Concurrency();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            CreateTask(1);
            CreateTask(2);
        }

        private void CreateTask(int taskId)
        {
            Task.Factory.StartNew(() =>
                {
                    for (int i = 0; i < 10; ++i)
                    {
                        int randomNumber = _random.Next(0, 50);

                        Console.WriteLine("Thread {0}, setting value {1}", taskId, randomNumber);
                        _concurrency.Value = randomNumber;
                        Console.WriteLine("Thread {0}, getting value {1}", taskId, _concurrency.Value);

                        Thread.Sleep(_random.Next(5, 15));
                    }
                });
        }
    }

The result is:

Thread 2, setting value 4
Thread 1, setting value 22
Thread 2, getting value 22
Thread 1, getting value 22
Thread 2, setting value 11
Thread 2, getting value 11
Thread 1, setting value 8
Thread 2, setting value 41
Thread 1, getting value 8
Thread 1, setting value 30
Thread 2, getting value 41
Thread 1, getting value 30
Thread 2, setting value 18
Thread 1, setting value 42
Thread 2, getting value 18
Thread 2, setting value 30
Thread 1, getting value 42
Thread 1, setting value 24
Thread 2, getting value 30
Thread 1, getting value 24
Thread 2, setting value 13
Thread 1, setting value 7
Thread 2, getting value 13
Thread 2, setting value 13
Thread 1, getting value 7
Thread 2, getting value 13
Thread 1, setting value 38
Thread 2, setting value 19
Thread 1, getting value 38
Thread 1, setting value 4
Thread 2, getting value 19
Thread 2, setting value 44
Thread 1, getting value 4
Thread 2, getting value 44
Thread 1, setting value 48
Thread 2, setting value 12
Thread 1, getting value 48
Thread 1, setting value 47
Thread 2, getting value 12
Thread 1, getting value 47

As you can see, everything is fine EXCEPT first setting/getting situation: Thread 2 sets value 2 but gets 22. And it's not single case, it happens every time. I know that setting and getting are not atomic and lock should be set around instructions in task, but why first attempt fails always and other work fine?

EDIT:

I updated Concurrency class to this:

private class Concurrency
        {
            private static Random _random = new Random();
            private int _myValue;
            private object _locker = new object();
            public int Value
            {
                set
                {
                    lock (_locker)
                    {
                        _myValue = value;
                        Thread.Sleep(_random.Next(5, 250));
                    }
                }

                get
                {
                    return _myValue;
                }
            }
        }

Note that I also expanded time range in Thread.Sleep. The result is:

Thread 2, setting value 3
Thread 1, setting value 9
Thread 2, getting value 9
Thread 2, setting value 44
Thread 1, getting value 9
Thread 1, setting value 35
Thread 2, getting value 44
Thread 2, setting value 32
Thread 1, getting value 35
Thread 1, setting value 25
Thread 2, getting value 32
Thread 2, setting value 15
Thread 1, getting value 25
Thread 1, setting value 5
Thread 2, getting value 15
Thread 2, setting value 34
Thread 1, getting value 5
Thread 1, setting value 42
Thread 2, getting value 34
Thread 2, setting value 36
Thread 1, getting value 42
Thread 1, setting value 8
Thread 2, getting value 36
Thread 2, setting value 42
Thread 1, getting value 8
Thread 1, setting value 16
Thread 2, getting value 42
Thread 2, setting value 0
Thread 1, getting value 16
Thread 1, setting value 43
Thread 2, getting value 0
Thread 2, setting value 20
Thread 1, getting value 43
Thread 1, setting value 30
Thread 2, getting value 20
Thread 2, setting value 38
Thread 1, getting value 30
Thread 1, setting value 0
Thread 2, getting value 38
Thread 1, getting value 0

Nothing changed really. I'm guessing that it's not matter of Random, but some other thing.

Try to pull out the for loop (from inside the task to the button1_Click routine)... — Adriano Repetti, Mar 04 '13 at 13:10

score 1 · Answer 1 · edited May 23 '17 at 12:21

1

It happens many times, not just the first one.

You "see" it just once and actually that is the bug in your program. Potentially each time you see two "setting..." you may read the last one. Imagine this situation:

Main Thread 1     Thread 2
Value = 0         
int x1 = Value     
                  Value = 2 
                  int x2 = Value
WriteLine(x1)     
                  WriteLine(x2)

Output is correct (0 for thread 1 and 2 for thread 2). Now imagine if scheduling is like this:

Main Thread 1     Thread 2
Value = 0         
                  Value = 2 
int x1 = Value     
WriteLine(x1)     
                  int x2 = Value
                  WriteLine(x2)

You'll get a wrong result because for both threads you'll read the value 2. Actually it's not wrong because the only operation locked is the set, there are no guarantees that the read operation (get of the property value) of thread 1 will be executed before the write operation (set of the property value) of thread 2.

Finally take a look to this post too, you'll see how code like that may fail (exactly for the same reason) if you write this:

++_concurrency.Value;

edited May 23 '17 at 12:21

Community

1
1

answered Mar 04 '13 at 12:44

Adriano Repetti

65,416
20
137
208

Ok, I already knew that and I'm aware that the lock placement is wrong. BUT, why - no matter how many times I run this application - only the first setting/getting fails, and others are ok? – Marek Kowalczyk Mar 04 '13 at 12:51
The "shared" seed of the Random object may contribute (after all there is a very small amount of time between the lock release and the property get) but the point is that you can't rely/study/examine instructions execution order of different threads. At least not at so high level. – Adriano Repetti Mar 04 '13 at 13:00
Probably (but for the reasons I said before it's just an useless guess) because first time you run the 2nd thread more time has been elapsed (because while you're creating the second task the first one is yet running). – Adriano Repetti Mar 04 '13 at 13:03

Matthew Watson · Accepted Answer · 2013-03-04T13:36:55.143

As you point out, the locking is incorrect. So it's more a question of "why does it seem to work, except at the start?". (I'm just restating your question.)

[EDIT]

Since you changed the code to remove the issue I was talking about, here's another idea - which I think really is the answer.

The way you have your code, there is an extremely short period of time between a thread exiting the lock and reading the value back.

Examine your setter:

set
{
    lock (_locker)
    {
        _myValue = value;
        Thread.Sleep(_random.Next(5, 25));
    }
}

Now if thread1 is inside the lock, it will set _myValue and then sleep. Thread2 in the meantime will be sitting waiting to enter the lock.

When thread1 exits the sleep, it immediately exits the lock and continues with the next line of code, which in this case is the line that prints the current value out:

Console.WriteLine("Thread {0}, getting value {1}", taskId, _concurrency.Value);

Unless thread1 is descheduled inbetween exiting the lock and dereferencing _concurrency.Value, it will receive the correct value. Because the time is so short, it is unlikely to be descheduled during that period.

If thread1 is descheduled, then thread2 would be able to enter the lock and change _myValue before thread1 dereferenced it.

Doing anything to increase the time between the thread setting and getting the value will make it more likely that an "incorrect" value will be observed.

Try the following program, and then uncomment the line indicated with // Try with this sleep uncommented.. You'll see a lot more lines printing "Number Mismatch".

using System;
using System.Threading;
using System.Threading.Tasks;


namespace Demo
{
    class Program
    {
        private static void Main(string[] args)
        {
            Console.WriteLine("Starting");
            CreateTask(1);
            CreateTask(2);
            Console.ReadKey();
        }

        private static void CreateTask(int taskId)
        {
            Task.Factory.StartNew(() =>
            {
                for (int i = 0; i < 10; ++i)
                {
                    int randomNumber = _random.Next(0, 50);

                    Console.WriteLine("Thread {0}, setting value {1}", taskId, randomNumber);
                    _concurrency.Value = randomNumber;
                    // Thread.Sleep(10); // Try with this sleep uncommented.
                    int test = _concurrency.Value;
                    Console.WriteLine("Thread {0}, getting value {1}", taskId, test);

                    if (test != randomNumber)
                    {
                        Console.WriteLine("Number mismatch.");
                    }

                    Thread.Sleep(_random.Next(5, 15));
                }
            });
        }

        private static Random _random = new Random();
        private static Concurrency _concurrency = new Concurrency();

    }

    class Concurrency
    {
        private int _myValue;
        private object _locker = new object();
        public int Value
        {
            set
            {
                lock (_locker)
                {
                    _myValue = value;
                    Thread.Sleep(_random.Next(5, 25));
                }
            }

            get
            {
                return _myValue;
            }
        }

        static Random _random = new Random();
    }
}

So why does it fail at the beginning? Well, I think that's just an artifact of the way the threads are started by the system.

Aah I missed that... I saw the shared `_random` in the `Form1` class and missed the bit where they were creating _new_ randoms. — Rawling, Mar 04 '13 at 12:58
You presented a good explanation of the reason why the lock statement is incorrect and I'm fully aware of this, but thanks anyway. Also additionally Sleep shows the problem. The only thing I didn't understand is this artifact you mentioned. I guess that resolves my question. — Marek Kowalczyk, Mar 04 '13 at 13:47

Why there is thread race at the beginning?

2 Answers2