4

I am a beginner in C++11 multithreading. I am working with small codes and came into this problem. Here is the code:

#include <iostream>
#include <thread>
#include <vector>
#include <mutex>

std::mutex print_mutex;

void function1()
{
    std::cout << "Thread1 started" << std::endl;

    while (true)
    {
        std::unique_lock<std::mutex> lock(print_mutex);
        for (size_t i = 0; i<= 1000000000; i++)
            continue;
        std::cout << "This is function1" << std::endl;
        lock.unlock();
    }
}

void function2()
{
    std::cout << "Thread2 started" << std::endl;
    while (true)
    {
        std::unique_lock<std::mutex> lock(print_mutex);
        for (size_t i = 0; i <= 1000000000; i++)
            continue;
        std::cout << "This is function2" << std::endl;
        lock.unlock();
    }
}

int main()
{
    std::thread t1(function1);
    std::thread t2(function2);

    t1.join();
    t2.join();

    return 0;
}

I have written code with the intuition of expecting the following output:

Thread1 started
Thread2 started

This is function1
This is function2
This is function1
. .
.
.

But the output shown is as follows:

Thread1 started
Thread2 started

This is function1

This is function1
This is function1
.
.
.

Where am I going wrong?

jeldikk
  • 353
  • 2
  • 12
  • Which compiler are you using? With Visual Studio 2013 the results are as expected. – Martin Schlott Apr 25 '15 at 13:56
  • Well, I don't think it's possible to predict how exactly will those threads be scheduled and therefore I think that the first output is perfectly valid. You should put that for cycle after unlock to get the desired output, but even then I think you won't be able to guarantee that you'll get always the same output. – hynner Apr 25 '15 at 14:07
  • I am using g++ 4.9.1 on ubuntu 14.10. – jeldikk Apr 25 '15 at 14:08
  • A little off topic, but worth mentioning are that the two `lock.unlock()` statements are harmless but completely unnecessary. The whole point of using a `std::unique_lock` is that it will automatically unlock its associated mutex when it goes out of scope. Also the two delay loops are probably going to get optimized away. You're better off using something like `std::this_thread::sleep_for()`. – Ferruccio Apr 25 '15 at 14:32
  • Ferruccio, That is a nice optimization tips for a beginner. is n't it that the lock gets switched to other waiting threads when the thread holding the lock goes to sleep ..?? – jeldikk Apr 25 '15 at 14:52
  • When a thread goes to sleep any mutexes it has locked will stay locked, but that's not what I was talking about. What I'm saying is that when the `lock` variable goes out of scope at the bottom of the while loop, its destructor will call its `unlock()` method, so it is unnecessary to call it explicitly. That's the guarantee that a `unique_lock` provides. You can lock it and know that it will be automatically unlocked when it goes out of scope. – Ferruccio Apr 25 '15 at 18:24

3 Answers3

4

Unlocking a mutex does not guarantee that another thread that's waiting to lock the same mutex will immediately acquire a lock.

It only guarantees that the other thread will TRY to acquire the lock.

In this case, after you unlock the mutex in one thread, the same thread will immediately try to lock it again. Even though another thread was waiting patiently, for the mutex, it's not a guarantee that the other thread will win this time. The same thread that just locked it can succeed in immediately locking it again.

Today, you're seeing that the same thread always wins the locking race. Tomorrow, you may find that it's always the other thread that does. You have no guarantees, whatsoever, which thread will acquire the mutex when there's more than one thread going after the same mutex, at the same time. The winner depends on your CPU and other hardware architecture, how busy the system is loaded, at the time, and many other factors.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
3

Both of your thread is doing following steps:

  • Lock
  • Long empty loop
  • Print
  • Unlock
  • Lock
  • Long empty loop
  • (and so on)

Practically, you haven't left any time for context switching, there is a lock just right after the unlock. Solution: Swap the "lock" and the "long empty loop" steps, so only the "print" step will be locked, the scheduler can switch to the other thread during "long empty loop".

Welcome to threads!

Edit: Pro Tipp: Debugging multithreading programs is hard. But sometimes it's worth to insert a plain printf() to indicate locks and unlocks (the right order: lock, then printf and printf then unlock), even when the program seems correct. In this case you could see the zero gap between unlock-lock.

ern0
  • 3,074
  • 25
  • 40
  • Swapping thing worked perfectly. As you said the scheduler needs time to switch the lock between threads, is this time predictable..?? because I need to do a double buffering using c++11. – jeldikk Apr 25 '15 at 14:17
  • You cannot rely on swapping the order of the loop and the lock to always work see [this](http://coliru.stacked-crooked.com/a/0dc3b5e2c7d0ad30) example – Scis Apr 25 '15 at 14:23
  • As the other answer says, you can't sure. As thumb of rule, you may lock things for only a very short time. Another good practice is, if the program makes it possible (e.g. it's not time critical), you may use sleep() between "rounds", say, 10-100-1000 ms, depends on the task (e.g. if you're watching a file to appear, use 1000 ms, if you're waiting for another task, use 100 ms sleep). – ern0 Apr 25 '15 at 14:24
1

This is a valid result, your code does not try to control the execution order in any way so as long as all threads execute at some point and there's is no problem and it's a legitimate result.

This could happen even if you switched the order of the loop and the lock(see here), because again you haven't written anything that attempts to control it using e.g conditional variables or just some silly atomic_bool(it is a silly solution just to demonstrate how can you actually make it alternating and be sure it will) boolean to alternate the runs.

Scis
  • 2,934
  • 3
  • 23
  • 37