Storing pointers to passed-by-reference objects in setStateCallback

[miguelprada]

I just spent a good while figuring out some segmentation faults I was getting in a very simple example using librFSM, and it all came down to confusion caused by StateMachine::setStateCallback taking StateCallback objects by reference and later storing pointers to them.

My use-case is more or less as follows. Say I have a generic StateCallback specialization (e.g. LoggingCb) which upon construction takes the name of each state (i.e. a single global won't work) and performs some logging on entry() and exit(). I want to add this callback to every state in the FSM, so therefore I do something like:

rfsm::StateMachine fsm;
fsm.load("some_file.lua");
for(auto& state : rfsm.getStateGraph().states)
{
  LoggingCb cb(state.name);
  fsm.setStateCallback(state.name, cb);
}
std::string *event;
while(event=getEvent()) // Get events from somewhere, not important
{
  fsm.sendEvent(*event);
  fsm.run(); // Segfault!
}

This quickly lead to segmentation faults, since every cb object created is shortly destroyed, but not before fsm has stored a pointer to it.

The way to avoid this issue is to do something like:

//...
for(auto& state : rfsm.getStateGraph().states)
{
  LoggingCb* cb(new LoggingCb(state.name));
  fsm.setStateCallback(state.name, *cb); // Eek! De-referencing a pointer to pass by reference
}
//...
// Don't forget to delete later

Which admittedly is not very nice.

In my experience, function parameters passed by non-const reference are usually considered output arguments, and the caller is not expected to keep the passed object alive.

Since the callee wants to store a pointer to the passed object, I think the function signature would signal this intention much more clearly by taking a pointer to a StateCallback instead of a reference, which would furthermore avoid the need to de-reference the pointer to pass the object by reference.

The next step, if you're willing to depend on C++11, TR1 or Boost, would be to take some sort of smart pointer. This would be the best solution in my opinion.

What do you think? Am I missing something important here?

---

Appendix

To make matters worse you can incorrectly try to fix the problem by directly storing the cb objects in a vector, e.g.:

//...
std::vector<LoggingCb> cbs;
for(auto& state : rfsm.getStateGraph().states)
{
  // Store first and THEN pass a reference to the value in the vector
  // because push_back will copy the value. Ha! I'm so smart!
  cbs.push_back(LoggingCb(state.name));
  fsm.setStateCallback(state.name, cbs.back());
}
//...

And this will (extremely likely) lead you to segmentation faults. The reason is that whenever the vector needs to reallocate memory, it will copy its existing contents to the newly allocated memory, thus invalidating any pointers taken so far. The result is that depending on how many states you have and the allocation policy of std::vector in your library, only some of the pointers stored inside StateMachine will remain valid.

Tell me this isn't a nice way to spend an afternoon chasing weird behavior :sweat_smile:

[miguelprada]

I've been discussing the issue with a colleague, and we've been looking at how this is done in other cases. Specifically, we've checked how ROS takes callback functions and they use three different possibilities:

1. take function pointers
2. take object pointers (actually they take member function pointers as well, but this is not necessary here since objects implementing a known interface are expected)
3. take copies of objects

In my original comment I was suggesting to go for the second approach, but the third could also be considered. I'd be happy to provide a PR if you wish, following option 2 or 3. The implications I see for both options are:

2. The signature of setStateCallback is changed, potentially breaking compatibility with client code. Documentation should clearly state that the caller is responsible of keeping the object alive longer than the state machine, and of deleting it later.
3. An implicit assumption is added to the API that the StateCallback object passed should be copyable, which should be noted in the documentation.

Note that currently there's also an implicit assumption that the instance passed must remain alive. Just documenting this is also an option, but then it's difficult (impossible?) to avoid the nasty issue of de-referencing dynamically allocated objects to be passed by reference.

[Nicogene]

Hi @miguelprada ,

thank you for investigating and reporting :wink: I don't like very much the option 3. because the copy will de-synchronize the stored data respect the original data:

Example:

class ExampleCallback : public rfsm::StateCallback {
public:
    virtual void entry() {
        std::cout<<"entry() of Configure (hello from C++)"<<std::endl;
    }

    virtual void doo() {
        if (5 == magicNum)
        {
            //do something 
        }
        else
        {
            //do something else
        } 
        std::cout<<"doo()   of Configure (hello from C++)"<<std::endl;
    }

    virtual void exit() {
        std::cout<<"exit()  of Configure (hello from C++)"<<std::endl;
    }
    void setMagicNumb(int num) 
    {
        magicNum = num; 
    } 
private:
    int magicNum;   
};

...

rfsm::StateMachine fsm;
fsm.load("some_file.lua");
ExampleCallback cb("state");
cb.setMagicNumb(1);
fsm.setStateCallback("state", cb);
cb.setMagicNumb(5);

internally the fsm will see the callback with the magicNum = 1, and not updated by the following cb.setMagicNumb.

I found the option 1 not very user friendly, we chose to use a class respect function pointer because it's easier to implement by the user.

Granted that I dislike breaking the API, probably the number 2 is the best option but I would like to listen the opinion of the designer of the library (@apaikan)