What happens if somebody tries to add a listener to a broadcaster while it is calling clear?
Jim > On Mar 10, 2015, at 10:30 AM, Pavel Labath <[email protected]> wrote: > > Hi clayborg, > > 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. > > http://reviews.llvm.org/D8211 > > Files: > source/Core/Broadcaster.cpp > source/Core/Listener.cpp > > Index: source/Core/Broadcaster.cpp > =================================================================== > --- source/Core/Broadcaster.cpp > +++ source/Core/Broadcaster.cpp > @@ -57,17 +57,21 @@ > void > Broadcaster::Clear() > { > - Mutex::Locker listeners_locker(m_listeners_mutex); > + 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 () > Index: source/Core/Listener.cpp > =================================================================== > --- source/Core/Listener.cpp > +++ source/Core/Listener.cpp > @@ -57,15 +57,20 @@ > void > Listener::Clear() > { > - Mutex::Locker locker(m_broadcasters_mutex); > - broadcaster_collection::iterator pos, end = m_broadcasters.end(); > - for (pos = m_broadcasters.begin(); pos != end; ++pos) > + broadcaster_collection old_broadcasters; > + { > + Mutex::Locker locker(m_broadcasters_mutex); > + // 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); > + 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); > - m_broadcasters.clear(); > - m_cond_wait.SetValue (false, eBroadcastNever); > - m_broadcasters.clear(); > - Mutex::Locker event_locker(m_events_mutex); > - m_events.clear(); > } > > uint32_t > > EMAIL PREFERENCES > http://reviews.llvm.org/settings/panel/emailpreferences/ > <D8211.21600.patch>_______________________________________________ > lldb-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits _______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
