Page MenuHomePhabricator

Fix deadlock in Listener/Broadcaster::Clear
AbandonedPublic

Authored by labath on Mar 10 2015, 10:30 AM.

Details

Reviewers
clayborg
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.

Diff Detail

Event Timeline

labath updated this revision to Diff 21600.Mar 10 2015, 10:30 AM
labath retitled this revision from to Fix deadlock in Listener/Broadcaster::Clear.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added a reviewer: clayborg.
labath added a subscriber: Unknown Object (MLST).

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

Jim

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 abandoned this revision.Mar 19 2015, 10:57 AM

abandoning as the race which prompted this was fixed elsewhere