boost::scoped_lock not working with local static variable?

Question

I made the following sample program to play with boost threading:

#pragma once
#include "boost\thread\mutex.hpp"
#include <iostream>

class ThreadWorker
{
public:
    ThreadWorker() {}
    virtual ~ThreadWorker() {}

    static void FirstCount(int threadId)
    {
        boost::mutex::scoped_lock(mutex_);
        static int i = 0;

        for(i = 1; i <= 30; i++)
        {
            std::cout << i << ": Hi from thread:  " << threadId << std::endl;
        }

    }

private:
    boost::mutex mutex_;
};

main class:

// ThreadTest.cpp
#include "stdafx.h"
#include "boost\thread\thread.hpp"
#include "ThreadWorker.h"

int _tmain(int argc, _TCHAR* argv[])
{
    boost::thread thread1(&ThreadWorker::FirstCount, 1);
    boost::thread thread2(&ThreadWorker::FirstCount, 2);
    boost::thread thread3(&ThreadWorker::FirstCount, 3);

    thread1.join();
    thread2.join();
    thread3.join();

    std::string input;
    std::cout << "Press <enter> to finish...\n";
    std::getline( std::cin, input );
    return 0;
}

When I run this I get the following output:

1: Hi from thread:  1
1: Hi from thread:  3
2: Hi from thread:  3
...

It looks like thread 1 gets there first and then thread 3. Isn't the scoped_lock supposed to prevent other threads from entering that section of code? Shouldn't the first thread that runs FirstCount() go to completion?

UPDATE

One thing I think is wrong with my code is this line:

boost::mutex::scoped_lock(mutex_);

I think it should be like:

boost::mutex::scoped_lock xyz(mutex_);

Once I do that, it does make the complaint about mutex_ not being static. Why it worked in the first place I'm not sure. Changing mutex_ to static does gives me a linking error:

1>ThreadWorker.obj : error LNK2001: unresolved external symbol "private: static class boost::mutex ThreadWorker::mutex_" (?mutex_@ThreadWorker@@0Vmutex@boost@@A) 1>c:\something\ThreadTest\Debug\ThreadTest.exe : fatal error LNK1120: 1 unresolved externals

Still playing with it.

Answer 1

You have two errors:

First of all, as already noticed, mutex_ should be static also:

private:
    static boost::mutex mutex_;

and of course declare it somewhere (in a .cpp file preferably!):

boost::mutex ThreadWorker::mutex_{};

Now, why does the compiler not complain? Well, because you actually do not construct a scoped lock with argument mutex_ here:

boost::mutex::scoped_lock(mutex_);

Actually this will not call the constructor that you want, but create a (local) object mutex_ that is of type scoped_lock and is constructed by the default constructor. Hence, no compiler issues. You should change it to something like the following:

boost::mutex::scoped_lock l{mutex_};

Now the compiler should start complaining about mutex_

Answer 2

You have three separate objects, and neither of them can see the other's mutex_ because that member is created within each object.

Perhaps you meant to make mutex_ static as well?

Edit: If I make the mutex static and remove the static from the i variable then it appears to work as I guess you meant it. Seems to be something like this happening: each thread enters the loop immediately, and are not locked from each other due to the mutex not being static. By the time they have all output to the console (I cannot remember whether there is a mutual exclusion on writing to cout) and i gets incremented they all see the static i as 30 and exit.

Edit 2: Nope, still not correct as there are still interspersed values on some runs.

Edit 3: The reason it compiles is that your scoped_lock is a temporary which seems to throw the compiler off the fact that mutex_ should be static. Try the following code instead:

#include <iostream>
#include "boost\thread\mutex.hpp"
#include "boost\thread\thread.hpp"

class ThreadWorker
{
public:
    ThreadWorker() {}
    virtual ~ThreadWorker() {}

    static void FirstCount(int threadId)
    {
        // Created object f here rather than temprary
        boost::mutex::scoped_lock f(mutex_);
        int i = 0; // Not static

        for(i = 1; i <= 30; i++)
        {
            std::cout << i << ": Hi from thread:  " << threadId << std::endl;
        }

    }

private:
    static boost::mutex mutex_; // Static
};

// Storage for static
boost::mutex ThreadWorker::mutex_;

int main(int argc, char* argv[])
{
    boost::thread thread1(&ThreadWorker::FirstCount, 1);
    boost::thread thread2(&ThreadWorker::FirstCount, 2);
    boost::thread thread3(&ThreadWorker::FirstCount, 3);

    thread1.join();
    thread2.join();
    thread3.join();

    std::string input;
    std::cout << "Press <enter> to finish...\n";
    std::getline( std::cin, input );
    return 0;
}

Answer 3

Does this code also compile with the same compiler?

Can you see the problem with this code? Try to spot the problem without compiling it.

class C {
public:
    static int f () {
        return i;
    }

    int i;
};

int main() {
    return C::f();
}

UPDATE: I just read your update.

class C {
public:
    static int f () {
        return i;
    }

    static int i;
};

int C::i = whatever;

int main() {
    return C::f();
}

Answer 4

Changed my code to be:

ThreadWorker.h:

#pragma once
#include "boost\thread\mutex.hpp"
#include <iostream>

class ThreadWorker
{ 
public:
    ThreadWorker();
    virtual ~ThreadWorker();

    static void FirstCount(int threadId);

private:
    static boost::mutex mutex_;
};

ThreadWorker.cpp:

#include "stdafx.h"
#include "ThreadWorker.h"

boost::mutex ThreadWorker::mutex_;

ThreadWorker::ThreadWorker()
{
}

ThreadWorker::~ThreadWorker()
{
}

void ThreadWorker::FirstCount(int threadId)
{
    boost::mutex::scoped_lock xyz(mutex_);
    static int i = 0;

    for(i = 1; i <= 30; i++)
    {
        std::cout << i << ": Hi from thread:  " << threadId << std::endl;
    }

}

ThreadTest.cpp:

// ThreadTest.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "boost\thread\thread.hpp"
#include "ThreadWorker.h"

int _tmain(int argc, _TCHAR* argv[])
{
    boost::thread thread1(&ThreadWorker::FirstCount, 1);
    boost::thread thread2(&ThreadWorker::FirstCount, 2);
    boost::thread thread3(&ThreadWorker::FirstCount, 3);

    thread1.join();
    thread2.join();
    thread3.join();

    std::string input;
    std::cout << "Press <enter> to finish...\n";
    std::getline( std::cin, input );
    return 0;
}

The change I made was 1. separating the header and cpp file (I originally only did this to make my post more compact) 2. giving the scoped_lock variable an identifier and 3. making the mutex static.

It now works as expected, printing 30 lines from each thread sequentially. What still baffles me is why the code compiled before (as tinman was also able to compile my original code).