util: Signal: make handler self-disconnection safer
Change-Id: I15eeed19af2da23187d7700ee7729f397491eca0
Refs: #2333
diff --git a/src/util/signal-signal.hpp b/src/util/signal-signal.hpp
index 4364d6a..ca6dcf1 100644
--- a/src/util/signal-signal.hpp
+++ b/src/util/signal-signal.hpp
@@ -128,11 +128,12 @@
/** \brief the disconnect function which will disconnect this handler
*
- * This is .disconnect method bound with the iterator to this slot.
+ * In practice this is the Signal::disconnect method bound to an iterator
+ * pointing at this slot.
*
* This is the only shared_ptr to this function object.
* Connection class has a weak_ptr which references the same function object.
- * When the slot is removed or the signal is destructed, this function object would be
+ * When the slot is erased or the signal is destructed, this function object is
* destructed, and the related Connections cannot disconnect this slot again.
*/
shared_ptr<function<void()>> disconnect;
@@ -170,7 +171,7 @@
}
template<typename Owner, typename ...TArgs>
-inline Connection
+Connection
Signal<Owner, TArgs...>::connect(const Handler& handler)
{
typename SlotList::iterator it = m_slots.insert(m_slots.end(), {handler, nullptr});
@@ -180,7 +181,7 @@
}
template<typename Owner, typename ...TArgs>
-inline Connection
+Connection
Signal<Owner, TArgs...>::connectSingleShot(const Handler& handler)
{
typename SlotList::iterator it = m_slots.insert(m_slots.end(), {nullptr, nullptr});
@@ -196,60 +197,72 @@
}
template<typename Owner, typename ...TArgs>
-inline void
+void
Signal<Owner, TArgs...>::disconnect(typename SlotList::iterator it)
{
- // it could be const_iterator, but gcc 4.6 doesn't support std::list::erase(const_iterator)
+ // 'it' could be const_iterator, but gcc 4.6 doesn't support std::list::erase(const_iterator)
if (m_isExecuting) {
// during signal emission, only the currently executing handler can be disconnected
- BOOST_ASSERT_MSG(it == m_currentSlot,
- "cannot disconnect another handler from a handler");
- m_currentSlot = m_slots.end(); // prevent disconnect twice
+ BOOST_ASSERT_MSG(it == m_currentSlot, "cannot disconnect another handler from a handler");
+
+ // this serves to indicate that the current slot needs to be erased from the list
+ // after it finishes executing; we cannot do it here because of bug #2333
+ m_currentSlot = m_slots.end();
+
+ // expire all weak_ptrs, to prevent double disconnections
+ it->disconnect.reset();
}
- m_slots.erase(it);
+ else {
+ m_slots.erase(it);
+ }
}
template<typename Owner, typename ...TArgs>
-inline bool
+bool
Signal<Owner, TArgs...>::isEmpty() const
{
return !m_isExecuting && m_slots.empty();
}
template<typename Owner, typename ...TArgs>
-inline void
+void
Signal<Owner, TArgs...>::operator()(const TArgs&... args)
{
BOOST_ASSERT_MSG(!m_isExecuting, "cannot emit signal from a handler");
+
if (m_slots.empty()) {
return;
}
- m_isExecuting = true;
- typename SlotList::iterator it = m_slots.begin();
- typename SlotList::iterator last = m_slots.end();
- --last;
+ auto it = m_slots.begin();
+ auto last = std::prev(m_slots.end());
+ m_isExecuting = true;
try {
bool isLast = false;
while (!isLast) {
m_currentSlot = it;
isLast = it == last;
- ++it;
m_currentSlot->handler(args...);
+
+ if (m_currentSlot == m_slots.end())
+ it = m_slots.erase(it);
+ else
+ ++it;
}
}
catch (...) {
m_isExecuting = false;
throw;
}
+
m_isExecuting = false;
}
template<typename Owner, typename ...TArgs>
-inline void
+void
Signal<Owner, TArgs...>::operator()(const TArgs&... args, const DummyExtraArg&)
{
this->operator()(args...);
diff --git a/tests/unit-tests/util/signal.t.cpp b/tests/unit-tests/util/signal.t.cpp
index 3b263b4..c06c4ac 100644
--- a/tests/unit-tests/util/signal.t.cpp
+++ b/tests/unit-tests/util/signal.t.cpp
@@ -381,15 +381,13 @@
int hit = 0;
Connection connection;
BOOST_CHECK_EQUAL(connection.isConnected(), false);
- connection = so.sig.connect(bind([] (int& hit, SignalOwner0& so, Connection& connection) {
+ connection = so.sig.connect([&so, &connection, &hit] {
++hit;
BOOST_CHECK_EQUAL(connection.isConnected(), true);
connection.disconnect();
BOOST_CHECK_EQUAL(connection.isConnected(), false);
BOOST_CHECK_EQUAL(so.isSigEmpty(), false); // disconnecting hasn't taken effect
- }, ref(hit), ref(so), ref(connection)));
- // Bug 2302, 2523: variables needs to be bound to the handler;
- // lambda capture won't work because closure would be destructed at .disconnect
+ });
so.emitSignal(sig);
BOOST_CHECK_EQUAL(hit, 1); // handler called