Multi-threaded game loop

This is a start on a multi-threaded game loop and I just wish to confirm my code is going in the correct direction, and if not, if there is any way it can be improved. I have good experience in game development and C++ in general, but limited experience when it comes to multi-threaded environments.

#include 
#include 
#include 
#include 
#include 
#include 

class Test
{
public:
    Test()
    {
        std::cout (new std::thread(&Test::renderFunc, this));
        updateThread = std::unique_ptr(new std::thread(&Test::updateFunc, this));
        std::cout join();
        updateThread->join();
        std::cout  renderThread;
    std::unique_ptr updateThread;
};

int main(int argc, char* argv[])
{
    std::unique_ptr test = std::unique_ptr(new Test());

    typedef std::chrono::high_resolution_clock Clock;
    typedef std::chrono::seconds Seconds;

    Clock::time_point t0, t1;
    Seconds seconds;

    t0 = Clock::now();
    do
    {
        t1 = Clock::now();
        seconds = std::chrono::duration_cast(t1 - t0);
    }while(seconds.count() quit();

    return 0;
}

(The timer code is just there for testing the threaded code.)

Yes you have added enough explicit locks to make it safe (under normal situations).

But it is not exception safe and not written in C++ style. You are writing the code as if it was Java (maybe). Which is causing all sorts of problems.

Why do you need the start() and stop() functions? They provide no utility and they initialize members that should be initialized in the constructor. Also the threads are started before the object is fully initialized and thus you have a thread running in a function when the object is not initialized correctly (and thus will probably immediately exit).

void start()
    {
        // Soon as you create a thread 
        // It starts running. So the next thing that is going to be
        // executed is the function `renderFunc()` which also depends
        // on member 'running' which has not been set.
        renderThread = std::unique_ptr(new std::thread(&Test::renderFunc, this));
        updateThread = std::unique_ptr(new std::thread(&Test::updateFunc, this));
        std::cout << "Threads Created" << std::endl;

        // You already have running threads that depend on this variable.
        // Yet this is the first time you even set it. Your code is already
        // broken.
        running = true;
    }

The next thing is the dynamic creation of the thread objects. Why. There is no need to dynamically create them. They should be automatic variables (ie non pointer members). The problem is that you need to be very careful with the initialization order.

class Test
{
    volatile bool running;
    std::mutex    mutex;
    std::thread   renderThread;
    std::thread   updateThread;
  public:
    Test()
        : running(false) // Make sure this is first.
        , renderThread(&Test::renderFunc, this)
        , updateThread(&Test::updateFunc, this)
    {
        std::cout << "Test starting" << std::endl;
    }
    ~Test()
    {
        std::cout << "Test finished" << std::endl;
        updateThread.join();
        renderThread.join();
    }
    inline void quit()
    {
        running = false;
    }

Next thing is your locking and unlocking of the mutex.

void updateFunc()
    {
        while(running)
        {
            mutex.lock();
            std::cout << "update" << std::endl;
            mutex.unlock();
        }
    }

This is not exception safe. Any situation were you do:

{
    lock(x);
    work();
    unlock(x);
}

Where lock/unlock is a combination that must be called as a pair. Then you should be using RAII. This is where you have an object where lock() is called in the constructor and unlock() is called in the destructor. This also makes the code exception safe because the destructor will always be called even if their are exceptions.

{
    LockObject  locker(x);  // calls lock(x)
    work();
}                           // destructor of locker will call unlock(x)

For this simple situation there is no need to create your own type. There is one defined in the standard scoped locked.

void updateFunc()
    {
        while(running)
        {
            std::lock_guard locker(mutex);
            std::cout << "update" << std::endl;
        }
    }

Stop using dynamic allocation when you do not need it:

std::unique_ptr test = std::unique_ptr(new Test());

    // Just do this:
    Test    test;

StackExchange Code Review Q#18817, answer score: 7