diff --git a/events/include/ignition/common/Event.hh b/events/include/ignition/common/Event.hh index ca1dfbe2a..d1097613b 100644 --- a/events/include/ignition/common/Event.hh +++ b/events/include/ignition/common/Event.hh @@ -156,8 +156,9 @@ namespace ignition private: class EventConnection { /// \brief Constructor - public: EventConnection(const bool _on, const std::function &_cb) - : callback(_cb) + public: EventConnection(const bool _on, const std::function &_cb, + const ConnectionPtr &_publicConn) + : callback(_cb), publicConnection(_publicConn) { // Windows Visual Studio 2012 does not have atomic_bool constructor, // so we have to set "on" using operator= @@ -169,6 +170,11 @@ namespace ignition /// \brief Callback function public: std::function callback; + + /// \brief A weak pointer to the Connection pointer returned by Connect. + /// This is used to clear the Connection's Event pointer during + /// destruction of an Event. + public: std::weak_ptr publicConnection; }; /// \def EvtConnectionMap @@ -197,6 +203,16 @@ namespace ignition template EventT::~EventT() { + // Clear the Event pointer on all connections so that they are not + // accessed after this Event is destructed. + for (auto &conn : this->connections) + { + auto publicCon = conn.second->publicConnection.lock(); + if (publicCon) + { + publicCon->event = nullptr; + } + } this->connections.clear(); } @@ -211,8 +227,10 @@ namespace ignition auto const &iter = this->connections.rbegin(); index = iter->first + 1; } - this->connections[index].reset(new EventConnection(true, _subscriber)); - return ConnectionPtr(new Connection(this, index)); + auto connection = ConnectionPtr(new Connection(this, index)); + this->connections[index].reset( + new EventConnection(true, _subscriber, connection)); + return connection; } /// \brief Get the number of connections. @@ -234,6 +252,14 @@ namespace ignition if (it != this->connections.end()) { it->second->on = false; + // The destructor of std::function seems to crashes if the function it + // points to is in a shared library and has been unloaded by the time + // the destructor is invoked. It's not clear whether this is a bug in + // the implementation of std::function or not. To avoid the crash, + // we call the destructor here by setting `callback = nullptr` because + // it is likely that EventT::Disconnect is called before the shared + // library is unloaded via Connection::~Connection. + it->second->callback = nullptr; this->connectionsToRemove.push_back(it); } } diff --git a/events/src/Event_TEST.cc b/events/src/Event_TEST.cc index 9a64f9203..388bd6b0d 100644 --- a/events/src/Event_TEST.cc +++ b/events/src/Event_TEST.cc @@ -283,6 +283,21 @@ TEST_F(EventTest, typeid_test) EXPECT_NE(typeid(Event3).name(), typeid(Event2).name()); } +///////////////////////////////////////////////// +TEST_F(EventTest, DestructionOrder) +{ + auto evt = std::make_unique>(); + common::ConnectionPtr conn = evt->Connect(callback); + evt->Signal(); + evt.reset(); + // Sleep to avoid warning about deleting a connection right after creation. + IGN_SLEEP_MS(1); + + // Check that this doesn't segfault. + conn.reset(); + SUCCEED(); +} + ///////////////////////////////////////////////// int main(int argc, char **argv) {