unique_ptr state object lifetime in state machine when pushing new state in enter method

Question

I am creating a state machine few states of which are switching the machine into a new (next) state in enter() method. The states are a unique_ptr objects which are created as needed and given to the machine.

As some states switch to the next state in enter() method (thus, while being used) I am concerned about possible problems here - when the current state calls set_state() the state machine assigns a new state to its state member thus losing the only pointer to the state from which this call was made.

Below is an example of my concern - the only unique_ptr for obj A and B is set to point to C while recursively calling this action from A and then B. Is this reliable? Will it cause any problems?

#include <memory>
#include <iostream>

class IState;
class IStateMachine {
public:
    virtual ~IStateMachine() = default;
    virtual void set_state(std::unique_ptr<IState> new_state) = 0;
};

class IState {
public:
    virtual ~IState() = default;
    virtual void enter(IStateMachine&) = 0;
};

class StateC : public IState {
public:
    void enter(IStateMachine&) override {
        std::cout <<  __func__ << ": StateC - start" << std::endl;
        std::cout <<  __func__ << ": StateC - end" << std::endl;
    }
};

class StateB : public IState {
public:
    void enter(IStateMachine& sm) override {
        std::cout <<  __func__ << ": StateB - start" << std::endl;
        sm.set_state(std::make_unique<StateC>());
        std::cout <<  __func__ << ": StateB - end" << std::endl;
    }
};

class StateA : public IState {
public:
    void enter(IStateMachine& sm) override {
        std::cout <<  __func__ << ": StateA - start" << std::endl;
        sm.set_state(std::make_unique<StateB>());
        std::cout <<  __func__ << ": StateA - end" << std::endl;
    }
};

class StateMachine : public IStateMachine {
public:
    void start() {
        set_state(std::make_unique<StateA>());
    }

    void set_state(std::unique_ptr<IState> new_state) {
        state_ = std::move(new_state);
        state_->enter(*this);
    }

    std::unique_ptr<IState> state_;
};

int main()
{
    StateMachine sm;
    sm.start();
}

The output from the code:

enter: StateA - start
enter: StateB - start
enter: StateC - start
enter: StateC - end
enter: StateB - end
enter: StateA - end

Edit 1:

The main idea behind is that the states are created as needed and automatically destroyed after they are not needed anymore. The restriction is that a state may do some work in its enter() method and then in the end of that method switch the state machine to the next state.

Edit 2:

I am not deleting the object explicitly. My question is more about lifetime of the object pointed to by a unique_ptr in case when a new object assigned to this one unique_ptr from the object`s own method (recursively) (see example code).

officially its undefined behaviour but as you don't access any members after deleting it will probably work on most platforms — Alan Birtles, Apr 10 '19 at 19:49
`this` dangles after the call to `sm.set_state`, which by itself is not UB. But referencing anything inside the class while `this` dangles will be UB. In any case looks like bad design. — rustyx, Apr 10 '19 at 19:53
I would really appreciate if you could explain more about bad design here, and maybe suggest a way(s) to improve. I added my intention to the bottom of the question. — Kostiantyn Ponomarenko, Apr 10 '19 at 20:04
Possible duplicate of [Is delete this allowed?](https://stackoverflow.com/questions/3150942/is-delete-this-allowed) — Michael Veksler, Apr 10 '19 at 20:20
I don't think it is a duplicate and I've added why I think so to this question. — Kostiantyn Ponomarenko, Apr 10 '19 at 20:31
You could make set_state() return the previous state. The caller can then decides its own lifetime. — Michael Doubez, Apr 10 '19 at 21:03
*"I don't think it is a duplicate "*. I don't see a difference between explicit delete this, or deletion through unique_ptr. Some of the answers in [is delete this allowed](https://stackoverflow.com/questions/3150942/is-delete-this-allowed) talk about that too — Michael Veksler, Apr 10 '19 at 21:06

score 0 · Answer 1 · answered Apr 10 '19 at 21:02

You could try adding a intermediary step:

In the enter() methods, instead of directly pushing a new state to your state machine (which deletes the current state that is actually calling for your machine to change state), you could tell the machine what kind of state you want to push. This would make a sort of pending push waiting to be delt with.

To do so, you could have an enum and a map associating each member of the enum with the actual state. Then you could deal with the pending push looking at what state you want to push directly in the state machine.

But to do so, it involves comming back to the machine again, so the states can't do all the work on their own.

It would look about something like that:

enum States
{
    State_A,
    //...
};

class StateMachine : public IStateMachine {
public:
    StateMachine () {
        fabric[State_A] = [] () { return std::unique_ptr<IState>(new StateA()); }
       // ....
    }

    void push(States state) { 
        pendingState_ = std::move(state); 
    }

    void update()
    {
        auto found = fabric_.find(pendingState_);
        assert(found != fabric_.end());
        state_ = found->second();
        state_->enter(*this);
    }

   // ... same as before ...
private:
    States                  pendingState_;
    std::map<States, std::function<std::unique_ptr<IState>()> > fabric_;
};

now in the enter methods, it would be :

sm.push(/*a state*/);

and in the main, you make it roll in a loop. So that means you also need to implement a way to check when the job is done, e.g. you could make a member of the enum None and check if the state of the machine is none or not.

unique_ptr state object lifetime in state machine when pushing new state in enter method

1 Answers1