Fix deadlock in Listener/Broadcaster::Clear

[labath]

Summary

When calling Listener::Clear() and Broadcaster::Clear() simultaneously from two threads a
deadlock would occur. Each object would obtain its internal lock (m_listeners_mutex,
m_broadcasters_mutex) and then proceed to deregister itself from the other object by calling
Broadcaster::RemoveListener() and Listener::BroadcasterWillDestruct() respectively. These
functions would attempt to obtain these locks again, resulting in a deadlock.

I resolve this issue my moving the lists of broadcasters and listeners to a local variable in the
Clear() function, so that RemoveListener() and BroadcasterWillDestruct() can be called without
holding the internal lock. Strictly speaking, doing this in only one function would be
sufficient, but I modified both for added safety.

[jingham]

What happens if somebody tries to add a listener to a broadcaster while it is calling clear?

Jim

[labath]

Good point.

In that case, I see the situation as follows:

a) If thread A calls broadcaster.Clear(), while thread B tries to add a new listener to the broadcaster then two things can happen:

a.i) AddListener gets to execute before the critical section in Clear(): in this case the listener will get added and immediately removed. This is no different from the situation from before the change.
a.ii) AddListener gets to execute after the critical section in Clear(): in this case the listener gets added and stays there. This is also the same behavior.

In either case the situation looks racy (did we want B to be stay registered? or did we want it removed?), and should be resolved by some synchronization between the threads A and B.

b) It get's weirder if thread B tries to add a listener which is already present in the broadcaster. In this case it could happen that the listener is registered in AddListener in thread B and subsequently deregistered by the thread A, even though it will still end up being present in the m_listeners list.

This is not really nice, but I would say that this code was buggy in the first place because it has no way of predicting the end result without additional synchronization. But it is true that without this patch, the end state would at least be completely consistent in any case.

I'll see if I can figure out a way to achieve this without the potential of deadlocking, because calling Clear() from two threads has very well defined semantics and it should not deadlock. (I have seen this deadlock in real life, although it was only exposed by an additional bug).

[labath]

abandoning as the race which prompted this was fixed elsewhere

source/Core/Broadcaster.cpp

Broadcaster::CheckInWithManager ()
     {
         m_manager->SignUpListenersForBroadcaster(*this);
     }
 }
 
 void
 Broadcaster::Clear()
 {
+    collection old_listeners;
+    {
         Mutex::Locker listeners_locker(m_listeners_mutex);
+        // Move the old listeners out of the way, so we can process them without holding the
+        // mutex.
+        old_listeners.swap(m_listeners);
+    }
     
     // Make sure the listener forgets about this broadcaster. We do
     // this in the broadcaster in case the broadcaster object initiates
     // the removal.
     
-    collection::iterator pos, end = m_listeners.end();
-    for (pos = m_listeners.begin(); pos != end; ++pos)
+    collection::iterator pos, end = old_listeners.end();
+    for (pos = old_listeners.begin(); pos != end; ++pos)
         pos->first->BroadcasterWillDestruct (this);
-    
-    m_listeners.clear();
 }
 const ConstString &
 Broadcaster::GetBroadcasterName ()
 {
     return m_broadcaster_name;
 }
 
 bool

source/Core/Listener.cpp

if (log)
         log->Printf ("%p Listener::~Listener('%s')",
                      static_cast<void*>(this), m_name.c_str());
     Clear();
 }
 
 void
 Listener::Clear()
 {
+    broadcaster_collection old_broadcasters;
+    {
         Mutex::Locker locker(m_broadcasters_mutex);
-    broadcaster_collection::iterator pos, end = m_broadcasters.end();
-    for (pos = m_broadcasters.begin(); pos != end; ++pos)
-        pos->first->RemoveListener (this, pos->second.event_mask);
-    m_broadcasters.clear();
+        // Move the old broadcasters out of the way, so we can process them without holding the
+        // mutex.
+        old_broadcasters.swap(m_broadcasters);
         m_cond_wait.SetValue (false, eBroadcastNever);
-    m_broadcasters.clear();
         Mutex::Locker event_locker(m_events_mutex);
         m_events.clear();
     }
 
+    broadcaster_collection::iterator pos, end = old_broadcasters.end();
+    for (pos = old_broadcasters.begin(); pos != end; ++pos)
+        pos->first->RemoveListener (this, pos->second.event_mask);
+}
+
 uint32_t
 Listener::StartListeningForEvents (Broadcaster* broadcaster, uint32_t event_mask)
 {
     if (broadcaster)
     {
         // Scope for "locker"
         // Tell the broadcaster to add this object as a listener
         {