[Lldb-commits] [PATCH] Fix deadlock in Listener/Broadcaster::Clear

[jingham at apple.com]

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 <labath at google.com> 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
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits