fix(AbstractEvent): operator-= holds the event mutex across Delegate:…

Foundation/include/Poco/AbstractEvent.h

@@ -184,8 +184,16 @@ class AbstractEvent
 		///
 		/// If the delegate is not found, this function does nothing.
 	{
-		typename TMutex::ScopedLock lock(_mutex);
-		_strategy.remove(aDelegate);
+		// Detach under the event mutex, disable() outside it. Holding
+		// the event mutex across Delegate::disable() would close a
+		// lock-order cycle with the M_delegate -> M_event edge that
+		// notify()'s target callbacks set up when they do `event -= d`.
+		typename TStrategy::DelegatePtr pRemoved;
+		{
+			typename TMutex::ScopedLock lock(_mutex);
+			pRemoved = _strategy.detach(aDelegate);
+		}
+		if (pRemoved) pRemoved->disable();
 	}
 
 	DelegateHandle add(const TDelegate& aDelegate)
@@ -206,8 +214,12 @@ class AbstractEvent
 		///
 		/// If the delegate is not found, this function does nothing.
 	{
-		typename TMutex::ScopedLock lock(_mutex);
-		_strategy.remove(delegateHandle);
+		typename TStrategy::DelegatePtr pRemoved;
+		{
+			typename TMutex::ScopedLock lock(_mutex);
+			pRemoved = _strategy.detach(delegateHandle);
+		}
+		if (pRemoved) pRemoved->disable();
 	}
 
 	void operator () (const void* pSender, TArgs& args)
@@ -305,8 +317,17 @@ class AbstractEvent
 	void clear()
 		/// Removes all delegates.
 	{
-		typename TMutex::ScopedLock lock(_mutex);
-		_strategy.clear();
+		// Same reason as operator -=: disable() must run outside _mutex.
+		typename TStrategy::Delegates detached;
+		{
+			typename TMutex::ScopedLock lock(_mutex);
+			detached = _strategy.detachAll();
+		}
+		for (typename TStrategy::Delegates::iterator it = detached.begin();
+		     it != detached.end(); ++it)
+		{
+			(*it)->disable();
+		}
 	}
 
 	bool empty() const
@@ -389,8 +410,13 @@ class AbstractEvent<void, TStrategy, TDelegate, TMutex>
 		///
 		/// If the delegate is not found, this function does nothing.
 	{
-		typename TMutex::ScopedLock lock(_mutex);
-		_strategy.remove(aDelegate);
+		// See the TArgs specialization for the rationale.
+		typename TStrategy::DelegatePtr pRemoved;
+		{
+			typename TMutex::ScopedLock lock(_mutex);
+			pRemoved = _strategy.detach(aDelegate);
+		}
+		if (pRemoved) pRemoved->disable();
 	}
 
 	DelegateHandle add(const TDelegate& aDelegate)
@@ -411,8 +437,12 @@ class AbstractEvent<void, TStrategy, TDelegate, TMutex>
 		///
 		/// If the delegate is not found, this function does nothing.
 	{
-		typename TMutex::ScopedLock lock(_mutex);
-		_strategy.remove(delegateHandle);
+		typename TStrategy::DelegatePtr pRemoved;
+		{
+			typename TMutex::ScopedLock lock(_mutex);
+			pRemoved = _strategy.detach(delegateHandle);
+		}
+		if (pRemoved) pRemoved->disable();
 	}
 
 	void operator () (const void* pSender)
@@ -503,8 +533,17 @@ class AbstractEvent<void, TStrategy, TDelegate, TMutex>
 	void clear()
 		/// Removes all delegates.
 	{
-		typename TMutex::ScopedLock lock(_mutex);
-		_strategy.clear();
+		// Same reason as operator -=: disable() must run outside _mutex.
+		typename TStrategy::Delegates detached;
+		{
+			typename TMutex::ScopedLock lock(_mutex);
+			detached = _strategy.detachAll();
+		}
+		for (typename TStrategy::Delegates::iterator it = detached.begin();
+		     it != detached.end(); ++it)
+		{
+			(*it)->disable();
+		}
 	}
 
 	bool empty() const

Foundation/include/Poco/DefaultStrategy.h

@@ -66,29 +66,59 @@ class DefaultStrategy: public NotificationStrategy<TArgs, TDelegate>
 	}
 
 	void remove(const TDelegate& delegate)
+	{
+		DelegatePtr p = detach(delegate);
+		if (p) p->disable();
+	}
+
+	void remove(DelegateHandle delegateHandle)
+	{
+		DelegatePtr p = detach(delegateHandle);
+		if (p) p->disable();
+	}
+
+	DelegatePtr detach(const TDelegate& delegate)
+		/// Removes a matching delegate from the internal list without
+		/// disabling it, and returns the detached delegate (or an empty
+		/// DelegatePtr if no match). The caller is responsible for
+		/// calling disable() on the returned delegate. This separation
+		/// allows AbstractEvent to call disable() outside its own mutex,
+		/// breaking the lock-order edge M_event -> M_delegate.
 	{
 		for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
 		{
 			if (delegate.equals(**it))
 			{
-				(*it)->disable();
+				DelegatePtr p = *it;
 				_delegates.erase(it);
-				return;
+				return p;
 			}
 		}
+		return DelegatePtr();
 	}
 
-	void remove(DelegateHandle delegateHandle)
+	DelegatePtr detach(DelegateHandle delegateHandle)
 	{
 		for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
 		{
 			if (*it == delegateHandle)
 			{
-				(*it)->disable();
+				DelegatePtr p = *it;
 				_delegates.erase(it);
-				return;
+				return p;
 			}
 		}
+		return DelegatePtr();
+	}
+
+	Delegates detachAll()
+		/// Removes all delegates from the internal list without disabling
+		/// them, and returns them so the caller can call disable() outside
+		/// its mutex. See detach().
+	{
+		Delegates out;
+		out.swap(_delegates);
+		return out;
 	}
 
 	DefaultStrategy& operator = (const DefaultStrategy& s)
@@ -102,11 +132,11 @@ class DefaultStrategy: public NotificationStrategy<TArgs, TDelegate>
 
 	void clear()
 	{
-		for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
+		Delegates detached = detachAll();
+		for (Iterator it = detached.begin(); it != detached.end(); ++it)
 		{
 			(*it)->disable();
 		}
-		_delegates.clear();
 	}
 
 	bool empty() const
@@ -164,29 +194,51 @@ class DefaultStrategy<void,TDelegate>: public NotificationStrategy<void, TDelega
 	}
 
 	void remove(const TDelegate& delegate)
+	{
+		DelegatePtr p = detach(delegate);
+		if (p) p->disable();
+	}
+
+	void remove(DelegateHandle delegateHandle)
+	{
+		DelegatePtr p = detach(delegateHandle);
+		if (p) p->disable();
+	}
+
+	DelegatePtr detach(const TDelegate& delegate)
+		/// See the TArgs specialization above.
 	{
 		for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
 		{
 			if (delegate.equals(**it))
 			{
-				(*it)->disable();
+				DelegatePtr p = *it;
 				_delegates.erase(it);
-				return;
+				return p;
 			}
 		}
+		return DelegatePtr();
 	}
 
-	void remove(DelegateHandle delegateHandle)
+	DelegatePtr detach(DelegateHandle delegateHandle)
 	{
 		for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
 		{
 			if (*it == delegateHandle)
 			{
-				(*it)->disable();
+				DelegatePtr p = *it;
 				_delegates.erase(it);
-				return;
+				return p;
 			}
 		}
+		return DelegatePtr();
+	}
+
+	Delegates detachAll()
+	{
+		Delegates out;
+		out.swap(_delegates);
+		return out;
 	}
 
 	DefaultStrategy& operator = (const DefaultStrategy& s)
@@ -209,11 +261,11 @@ class DefaultStrategy<void,TDelegate>: public NotificationStrategy<void, TDelega
 
 	void clear()
 	{
-		for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
+		Delegates detached = detachAll();
+		for (Iterator it = detached.begin(); it != detached.end(); ++it)
 		{
 			(*it)->disable();
 		}
-		_delegates.clear();
 	}
 
 	bool empty() const

Foundation/include/Poco/NotificationStrategy.h

@@ -19,6 +19,8 @@
 
 
 #include "Poco/Foundation.h"
+#include "Poco/SharedPtr.h"
+#include <vector>
 
 
 namespace Poco {
@@ -34,6 +36,8 @@ class NotificationStrategy
 {
 public:
 	using DelegateHandle = TDelegate*;
+	using DelegatePtr    = SharedPtr<TDelegate>;
+	using Delegates      = std::vector<DelegatePtr>;
 
 	NotificationStrategy() = default;
 
@@ -53,6 +57,23 @@ class NotificationStrategy
 		/// Removes a delegate from the strategy, if found.
 		/// Does nothing if the delegate has not been added.
 
+	virtual DelegatePtr detach(const TDelegate& delegate) = 0;
+		/// Removes a matching delegate from the strategy without disabling
+		/// it, and returns the detached delegate (or an empty DelegatePtr
+		/// if no match). The caller is responsible for calling disable()
+		/// on the returned delegate. AbstractEvent uses this to avoid
+		/// holding the event mutex across Delegate::disable() — see
+		/// AbstractEvent::operator -= for the lock-order rationale.
+
+	virtual DelegatePtr detach(DelegateHandle delegateHandle) = 0;
+		/// Same as detach(const TDelegate&), keyed by handle.
+
+	virtual Delegates detachAll() = 0;
+		/// Removes all delegates from the strategy without disabling them
+		/// and returns them, so the caller can disable() each outside its
+		/// own mutex. AbstractEvent::clear() uses this for the same reason
+		/// detach() exists.
+
 	virtual void clear() = 0;
 		/// Removes all delegates from the strategy.
 
@@ -71,6 +92,8 @@ class NotificationStrategy<void, TDelegate>
 {
 public:
 	using DelegateHandle = TDelegate*;
+	using DelegatePtr    = SharedPtr<TDelegate>;
+	using Delegates      = std::vector<DelegatePtr>;
 
 	NotificationStrategy() = default;
 
@@ -90,6 +113,15 @@ class NotificationStrategy<void, TDelegate>
 		/// Removes a delegate from the strategy, if found.
 		/// Does nothing if the delegate has not been added.
 
+	virtual DelegatePtr detach(const TDelegate& delegate) = 0;
+		/// See the TArgs specialization above.
+
+	virtual DelegatePtr detach(DelegateHandle delegateHandle) = 0;
+		/// See the TArgs specialization above.
+
+	virtual Delegates detachAll() = 0;
+		/// See the TArgs specialization above.
+
 	virtual void clear() = 0;
 		/// Removes all delegates from the strategy.