Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Below 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, is there 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 <iostream>
#include <string>
#include <thread>
#include <mutex>
#include <memory>
#include <chrono>

class Test
{
public:
    Test()
    {
        std::cout << "Test starting" << std::endl;
        start();
    }
    ~Test()
    {
        stop();
        std::cout << "Test finished" << std::endl;
    }
    inline void quit()
    {
        running = false;
    }
private:
    void start()
    {
        renderThread = std::unique_ptr<std::thread>(new std::thread(&Test::renderFunc, this));
        updateThread = std::unique_ptr<std::thread>(new std::thread(&Test::updateFunc, this));
        std::cout << "Threads Created" << std::endl;
        running = true;
    }
    void stop()
    {
        renderThread->join();
        updateThread->join();
        std::cout << "Threads joined" << std::endl;
    }

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

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

    volatile bool running;
    std::mutex mutex;
    std::unique_ptr<std::thread> renderThread;
    std::unique_ptr<std::thread> updateThread;
};

int main(int argc, char* argv[])
{
    std::unique_ptr<Test> test = std::unique_ptr<Test>(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<Seconds>(t1 - t0);
    }while(seconds.count() <= 15);

    test->quit();

    return 0;
}

Is this code a good starting point to continue with, or is there any major flaws that should be fixed before I continue. Thanks

P.S The timer code is just there for testing the threaded code

share|improve this question
    
Maybe I'm misunderstanding what's going on in your code but what's the point in having multiple threads if you're just going to have them wait on the same mutex? You've essentially made it single threaded since the threads can only execute sequentially. Right? –  Jeff Mercado Nov 20 '12 at 5:51
    
@Jeff Mercardo You may very well be right, like I said I have little to no experience with multithreaded programs, but most examples I have seen only used one mutex, but that may be down to the fact most were only reader/writer programs. Having two mutexs would probably be more beneficial but how would that ensure thread safety? –  const_ref Nov 20 '12 at 8:54
    
When I compile and run this I get an exception terminate called after throwing an instance of 'std::system_error' what(): Operation not permitted –  Loki Astari Nov 20 '12 at 13:49
    
Compiles and runs using Visual Studio 2012 for me. What compiler are you using? –  const_ref Nov 20 '12 at 14:03

1 Answer 1

up vote 5 down vote accepted

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<std::thread>(new std::thread(&Test::renderFunc, this));
        updateThread = std::unique_ptr<std::thread>(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<std::mutex> locker(mutex);
            std::cout << "update" << std::endl;
        }
    }

Stop using dynamic allocation when you do not need it:

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

    // Just do this:
    Test    test;
share|improve this answer
    
Very useful feedback, Thanks :) –  const_ref Dec 17 '12 at 8:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.