boost::signals2 thread-safe destruction

Question

I have a class that contains a boost::signals2::scoped_connection as a data member. This connection contains a slot which, when triggered, will print another data member in the class to stdout:

struct A {
   A(boost::signals2::signal<void()>& connection)
    : scoped_connection_{connection.connect([this]() {
          std::cout << foo_ << std::endl;
      })} {}

    std::string foo_{"foo"};
    boost::signals2::scoped_connection scoped_connection_;
};

This code can segfault in a multi-threaded scenario if a thread fires the signal (hence executing the lambda), but another thread destroys the object at the same time:

using Signal = boost::signals2::signal<void()>;

struct A {
    A(Signal& connection)
        : scoped_connection_{connection.connect([this]() {
              std::this_thread::sleep_for(std::chrono::seconds(5));
              // foo is destroyed at this point...
              std::cout << foo_ << std::endl; // segfault
          })} {}

    std::string foo_{"Kaboom"};
    boost::signals2::scoped_connection scoped_connection_;
};

int main() {
    Signal sig;
    auto a = std::make_unique<A>(sig);

    std::thread t([&sig]() { sig(); });

    // give the thread some time to start and signal the slots
    std::this_thread::sleep_for(std::chrono::seconds(1));

    // destroy a as the callback is executing...
    a.reset();
   
    t.join();
    return 0;
}

This code segfaults because the destruction of scoped_connection is not synchronized with the execution of the slot's callback. In other words scoped_connection can be destroyed even if the callback is currently running.

This was surprising behavior for me, I would have expected scoped_connection's destructor to block execution until the callback has finished.

Now I'm faced with the challenge of making above's code thread-safe by synchronizing destruction with callback execution and haven't been able to find a satisfactory solution. Maybe some of you can shed some light of a possible approach I can try out.

Here godbolt link reproducing the segfault

Answer 1

This was surprising behavior for me, I would have expected scoped_connection's destructor to block execution until the callback has finished.

The scoped_connection's release only synchronizes on invocation. This means that manipulating the signal doesn't create a data race. Whatever you share in the handler itself is your own responsibility.

So you could play fast and loose and "detect" that the slot was disconnected:

if (scoped_connection_.connected())
    std::cout << foo_ << std::endl;

However that's still intrinsically racy (though it will be very rare to hit, and probably never given the example timings).

---

Better, take shared ownership of the shared resource:

Live On Coliru

#include <boost/signals2.hpp>
#include <iostream>
#include <memory>
#include <thread>

using Signal = boost::signals2::signal<void()>;

struct A {
    A(Signal& sig)
        : scoped_connection_{sig.connect([s = state_] {
            std::cout << "Executing callback" << std::endl;
            std::this_thread::sleep_for(std::chrono::seconds(5));
            // foo is shared owned
            std::cout << s->foo_ << std::endl;
        })} {}

    ~A() { std::cout << "Destroying A" << std::endl; }

    struct State {
        std::string foo_;
        ~State() { std::cout << "Destroying shared state" << std::endl; }
    };
    std::shared_ptr<State>             state_ = std::make_shared<State>(State{"Naboo"});
    boost::signals2::scoped_connection scoped_connection_;
};

int main() {
    Signal sig;
    auto a = std::make_shared<A>(sig);  // register "a" as a slot
    std::thread t([&sig]() { sig(); });
    // give the thread some time to start and signal the slots
    std::this_thread::sleep_for(std::chrono::seconds(1));
    // destroy a as the callback is executing...
    a.reset();
    t.join();
}

Printing

Destroying shared state
Executing callback
Destroying A
Naboo
Destroying shared state

Of course the current example could be much simplified: https://coliru.stacked-crooked.com/a/8faf77323a390921 but I'm assuming your code represents more interesting code

Answer 2

First observation would be to pay a bit more overhead by switching to shared_ptr instead of unique_ptr:

struct A{
    A(Signal& sig):
        foo_{std::make_shared<std::string>("foo message")}
    {
        sig.connect([foo_]{
            std::this_thread::sleep_for(std::chrono::seconds(5));
            // foo is not destroyed at this point...
            std::cout << *foo_ << std::endl; // no segfault
        });
    };
    std::shared_ptr<std::string> foo_;
};

But this forces the object to live as long as the slot references it. To gain better control you need to track slot-bound objects:

using slot=signal::slot_type;

struct A{
    A(Signal& sig):
         data_{std::make_shared<slot_data>()}
    {
         data_->scoped_connection_ = sig.connect(slot{[&foo_=data_->foo_]{
              std::this_thread::sleep_for(std::chrono::seconds(5));
              // foo is life extended at this point...
              std::cout << foo_ << std::endl; // no segfault
          }}.track_foreign(data_));
    };
    struct slot_data{
        std::string foo_{"Kaboom"};
        boost::signals2::scoped_connection scoped_connection_;
    };

    std::shared_ptr<slot_data> data_;
};

By tracking the weak_ptr, slot checks if the shared object is in its valid lifetime, before execution; if the answer is yes, then the shared_ptr is life extended for the duration of slot execution; but if the answer is no, the slot is dropped from execution.

EDIT: Removed the enable_shared_from_this, because of impossiblity of using shared_from_this in constructor. This solution costs more than I would like to pay.