Using a std::function as a callback signal

[yassiezar]

At the moment, message callbacks are handled by defining a function to receive a message and create a boost signal by passing said function and its attached parent object. Is there any way to define the callback as a std::function variable instead and register it as a callback with the signal manager instead? This would make it a bit less verbose to write a simple 1-line functions that fires a state transition event, for example.

┆Issue is synchronized with this Jira Task by Unito
┆Link To Issue: https://robosoft-ai.atlassian.net/browse/SMACC1-3

[pabloinigoblasco]

I don't totally follow your original example about how it is done now.
Could you put around a link about an example of that case you describe? Are you talking about SmaccSignals?

Also, I am not sure if it is the case, but you could add a callback function to a transition execution, have a look to "react_with_action":
https://github.com/reelrbtx/SMACC/blob/master/smacc/include/smacc/smacc_transition.h

Another mechanism we have is a "labeled OnExit" callback. You could define some exit code if some specific transition was executed.

for example, something like:

void onExit(SUCESS)
{
 // whatever
}

[yassiezar]

I was mostly curious about making the callback registration with SmaccSignals, i.e. Boost.Signals2, less verbose. E.g. you currently create a callback like so:

void callback(const int& param) {
  // whatever
}

void onEntry() override {
  Client* client;
  this->requiresClient(client);    
  client->onMessageReceieved(&Object::callback, this);
}

I'm wondering if there's a way to do something like this instead:

void onEntry() override {
  Client* client;
  this->requiresClient(client);    
  client->onMessageReceieved([](const int& param){ /* whatever */ });
}

I tried doing this, but ran into compilation problems and I couldn't tell if this was a Boost.Signals2 limitation or not. This is purely a stylistic question, rather than functional. I only ask since we have a few 1-line callbacks which would be nice to replace with a lambda or std::function

[pabloinigoblasco]

I like the idea of lambda expressions, I am a fan of them.

But this simple example is not as simple as it may look and there his a reason why right now the syntax is like that.
This is a very interesting issue and shows one of the strengths of SMACC (IMHO). After I explain it we could discuss about how to improve that syntax.

Let me ask you first a question:
What would happen if the lambda expression capture local data of the state or the client behavior (short-life objects) and the client (a long-life object) receives a message after we left the state?

• the answer is: if you did not unsubscribed properly when you leave the state or the clientBehavior the application would perform a segmentation fault, because used objects in the callback are not in memory anymore.

SMACC implements an automatic unsubscription mechanism for callbacks (using the object SmaccSignal). So, a Smaccsignal is something more than a boostSignal.

The SMACC stateMachine contains a list of all signal connections and disconnect them when it is required.

Now, I would love to hear more about your thoughts taking all this information into account. I am not totally happy with the implementation of SmaccSignal, I think they should be easier to use. However, I also think this mechanism for auto-management of signal connections is important and should be kept.

[yassiezar]

That's a fair point and I've not though about it too much. I agree, SMACC's automatic signal management features are really one of its strong points.

As far as I know, lambda and std::function objects share the same lifetime as their container objects (same as a functor). So, in theory, the lambdas/std::function will be dropped like the functors are now when a short-lived object is dropped. Also, looking at the code, it doesn't seem that the state machine or signal handlers need an explicit handle to the object in order to disconnect or drop signals (see here). Is there something else going on that I'm missing? Because it seems that what we can do is make additional binder functions like these to accommodate std::function arguments (AFAIK lambdas are automatically transformed to std::function types, so its compatible with both arguments). Something like

template <>
struct Bind<1>
{
    template <typename TSmaccSignal, typename TMemberFunctionPrototype>
    boost::signals2::connection bindaux(TSmaccSignal &signal, TMemberFunctionPrototype callback)
    {
        return signal.connect(callback);
    }
};

This does seem suspiciously simple though, so curious to hear what I'm missing

[pabloinigoblasco]

I am afraid, it is not as easy as it may look. It would be great if you found a solution, but I do not see there is a solution yet.

The actual challenge is not making our code compile using a lambda, that is totally possible. In fact, that option already existed for some time but it was removed.

Lets talk about the issue with an example.

Lets say we have a client (long-life object) such as the Nav2ZClient.
This client has some signals such as onSuccess or onFailure that are invoked when we receive a ros2-action response.

Now, lets say we want to subscribe to some of these signals from some short-life object such as an state or a clientbehavior.
That is actually the usual case of the navigation client behaviors, lets say CbNavigateForward.

Actually CbNavigateForward wants to subscribe to Nav2ZClient's signals to post their own events.
It would be great if we could use a lambda here.

nav2zclient->onSuccess([this, = ]()
{
    this->fooCb();
});

The lambda function is owned by the nav2zclient, a long-term object. (it is not owned by the client behavior)

question: What would it happen if that onSuccess signal is raised after we left the state because some transition was triggered?
In that case the CbNavigateForward would not exist anymore, neither the state.

But the lambda would be invoked. We would get here a segmentation fault on this->fooCb().

This is what I see about using lambdas. Does that make sense for you? Maybe I am the one that is missing something about your view.

[yassiezar]

@pabloinigoblasco I've had some time to look at this again and reviving this thread from the dead (hope that's ok :) ). I was wondering if you could explain how SmaccSignals work and how they're different to boost::signals2 signals? I see that SmaccSignals are based off of boost::signals2, so was hoping you could clear up the difference.

I think the issue is that the signals aren't really attached to a specific object's lifetime. They're lifetimes are manually managed and are destroyed during state transitions here, but if (for whatever reason) a client behavior goes out of scope before a transition occurs, a segfault will also take place. Would it be possible to make this lifetime attachment using something like std::bind where the signals are created here? Would the caller be informed that the object is out of scope which could then trigger some kind of deletion?

Also, would it make sense to look at using signal2's track() method to automatically manage signals' lifetimes? This would make tracking the signals' lifetimes truly automatic, but would require considerable work to implement, because the objects will need to be encapsulated inside shared_ptr objects to be compatible with track. However, there might be additional benefits to this and using smart pointers more broadly (I mentioned a few in #86 (comment)).

Curious to hear your thoughts?

[yassiezar]

Hello @yassiezar. I think is very good to analyze together internal code. It is a way of documenting and making the core stronger. So thanks.

But if (for whatever reason) a client behavior goes out of scope before a transition occurs, a segfault will also take place.

The short-lifetime types we have are: states, client-behaviors, state-reactors and event generators.

The scope of all these types is controlled by internal SMACC code so this disconnection happens always before they are destroyed. Because of that "client behavior goes out of scope before a transition occurs" cannot happen. (unless there is a bug). Maybe I am missing something?

if we are talking about objects with more uncertain lifetimes, it may be worthwhile to focus on components or clients instead. Although, as of now, SMACC signals are designed to exist throughout the lifetime of the entire application, perhaps we should consider the possibility that this may not always be the case.

Would it be possible to make this lifetime attachment using something like std::bind where the signals are created here? Would the caller be informed that the object is out of scope which could then trigger some kind of deletion?

Also, would it make sense to look at using signal2's track() method to automatically manage signals' lifetimes? This would make tracking the signals' lifetimes truly automatic, but would require considerable work to implement, because the objects will need to be encapsulated inside shared_ptr<ISmaccClientBehaviour> objects to be compatible with track. However, there might be additional benefits to this and using smart pointers more broadly (I mentioned a few in #86).

I am open to introducing new changes to the syntax of SMACC signals. Initially, I had planned to create an extension of boost::signals2:signal that would be capable of handling the lifetime of objects and would use the same syntax. However, upon attempting this, I encountered several problems that ultimately led me to switch to the current syntax we are using.

Now, the question arises as to what would be the impact of transitioning from the current SMACC signals and pointers to the new version that uses shared_ptrs, which would require changing examples, libraries, and code.

I believe that such a transition would be a costly development effort, and therefore, I suggest that before taking any action, we should experiment with some motivating use cases and play with some code chunks that showed the benefits of the new final syntax.

---

@pabloinigoblasco I found a way to roll back my previous comment so I copied your response here. Feel free to post it again if you want

[yassiezar]

The scope of all these types is controlled by internal SMACC code so this disconnection happens always before they are destroyed. Because of that "client behavior goes out of scope before a transition occurs" cannot happen. (unless there is a bug). Maybe I am missing something?

When I started using SMACC, I thought that the signal connection lifetimes were attached to the parent object's lifetime. The robust attachment/detachment performance during high-frequency transitions and the fact you pass the parent's this to the connection call reinforced this idea. However, digging into the code and your above comment confirms that this isn't the case - there's a manual call to to detach a signal right before the CB goes out of scope.

It looks from the surface then that the main thing that prevents us using an anonymous function instead of a functor for the signal callbacks are that there are no objects to reference when you're doing the manual disconnections during state transitions.

I believe that such a transition would be a costly development effort, and therefore, I suggest that before taking any action, we should experiment with some motivating use cases and play with some code chunks that showed the benefits of the new final syntax.

Totally agreed, the benefits of transitioning to a new memory management approach needs to justify the development effort required. I've mentioned a couple of benefits of moving to smart pointers already, but I'm sure you'll have better insight on the problem.

I guess the question is whether there's interest from your and the other core maintainers' side to see a transition to smart pointers take place? I'd like to play around with a making minimum viable example when I have some spare time, but your input would of course be hugely helpful.