Search code examples
c++c++11architecturestdthread

Constructing object in new thread?


I'm trying to construct objects, that take a while to build in a seperate thread. (Later for loading game assets in real time, while the render loop is running :))

While the object is still building, requests to that object will not fail, but do something in replacement. (Like drawing without textures, while the texture is still loading, in the game). My approach to this is using the State-Pattern - one state for avaiable one for still loading. (My first one was with Proxy, but you really don't wanna see THAT code!)

Here's the complete source code:

#include <thread>
#include <iostream>
#include <list>
using namespace std;


class Object
{
private:
    int _data;
    /// Classes for states ///
    class IState
    {
    public:
        virtual void Use(Object *obj) = 0;
    };
    class Unavailable : public IState
    {
    public:
        void Use(Object *obj)
        {cout << "Object not avaiable..." << endl;}
    };
    class Avaiable : public IState
    {
    public:
        void Use(Object *obj)
        {cout << "Data is: " << obj->_data << endl;}
    };
    ////////////////////////
    IState *_state;
    void ChangeState(IState *newstate)
    {
        delete _state;
        _state = newstate;
    }
    void Construct() //Can this be part of IState?
    {
        this_thread::sleep_for(chrono::seconds(1)); //Work hard
        _data = 50;
        ChangeState(new Avaiable());
    }
public:
    Object()
    {
        //Set the state to unavaiable
        _state = new Unavailable();
        //Construct the object in seperate thread
        thread constructor(&Object::Construct, this); //How do I refer to Construct if its a member of IState?
        constructor.detach();
        //Thread runs while out of scope!
    } 
    ~Object()
    {delete _state;}

    void Use()
    {
        //Redirect actions
        _state->Use(this);
    }
};


int main()
{
    {
        list<Object*> objects;
        for (int i = 0; i < 10; i++)
        {
            this_thread::sleep_for(chrono::milliseconds(500));
            objects.push_back(new Object()); //I can't use Object as the list type, because of push_back()
                                             //copying the Object then DELETING it, thus deleting the state
            for (auto obj : objects) //The objects, which are already build shoud write "50"
                                     //otherwise it should write "Not avaiable" as a replacement
            {
                obj->Use();
            }
        }
        //Free the objects (I really want to avoid this....)
        for (auto obj : objects)
            delete obj;
    } //Extra scope to prevent false memory leaks!
    _CrtDumpMemoryLeaks(); //Reports memory leak somewhere. Couldn't track the block back to my code(...)
}

The questions are (as mentiont in the code):

  1. How could I move the Construct() method in the IState-interface in order to improve design (Discard Construct() calls, if the object is already avaiable!)
  2. Can I make a list of Object instead of Object*?
  3. What could that memory leak be? (I can't track the block back to my own code with _CrtSetBreakAlloc?!)

Looking forware to your answers and comments on design. (I just began to deal with software-architecture, so please correct me if this approach is rubbish ;))


Solution

  • That is not threadsafe, because if you call Object::Use and while it is in use, the construction in the other thread finishes, it deletes the dummy that is still being used in the main thread, leading to undefined behavior.

    I see two approaches right now:

    1 instead of a plain pointer, use a shared_ptr:

    class Object
    {
    private:
        int _data;
        std::shared_ptr<IState> _state;
        void ChangeState(std::shared_ptr<IState> newstate) {
          std::atomic_exchange(&_state, std::move(newstate));
        }
        void Construct() {
          //...
          ChangeState(std::make_shared<Avaiable>());
        }
    public:
        Object() : 
          _state{std::make_shared<Unavailable>()}
        {
          std::thread constructor(&Object::Construct, this); 
          constructor.detach();
        } 
        ~Object()
        {}
    
        void Use() {
          std::shared_ptr<IState> currentState = std::atomic_load(&_state); //!!
          currentState->Use(*this)
        }
    };
    

    The atomic_load of the state is important, it makes a copy of the current shared_ptr and the whole Use operation executes on the old state, even if it gets exchanged meanwhile. The destruction is then done in the main thread, after Use() has finished execution.

    2 use a future and actively look for the constructed object:

    class Object
    {
    private:
        int _data;
        typedef std::unique_ptr<IState> StatePtr;
        typedef std::future<StatePtr()> Constructor; 
    
        StatePtr _state;
        Constructor _constructor
    
        StatePtr  Construct() {
          //...
          return std::make_unique<Avaiable>();
        }
    
        static bool isReady(Constructor& ctor) {
          return ctor.valid()
            && ctor.wait_for(std::chrono::seconds(0)) != std::future_status::timeout;
        }
    
        IState& getState() {
          if (isReady(_constructor)) {
            _state = _constructor.get();
            _constructor.reset();
          }
          return *_state;
        }
    
       
    public:
        Object() : 
          _state{std::make_unique<Unavailable>()} ,
          _constructor{ std::async(std::launch::async, [this](){Construct();} )}
        {} 
    
        void Use() {
          getState().Use(*this);
        }
    };