Fix a race in Broadcaster/Listener interaction
authorPavel Labath <labath@google.com>
Mon, 15 Aug 2016 09:53:08 +0000 (09:53 +0000)
committerPavel Labath <labath@google.com>
Mon, 15 Aug 2016 09:53:08 +0000 (09:53 +0000)
commit8749089c8c47eccdaad976647b8511c833959507
treedb5e53575b6ab4c7d69e0e16a982bd765bafba52
parentb6f1bb13aec0d00ecb6f8ff34b22be4b972914fe
Fix a race in Broadcaster/Listener interaction

Summary:
The following problem was occuring:
- broadcaster B had two listeners: L1 and L2 (thread T1)
- (T1) B has started to broadcast an event, it has locked a shared_ptr to L1 (in
  ListenerIterator())
- on another thread T2 the penultimate reference to L1 was destroyed (the transient object in B is
  now the last reference)
- (T2) the last reference to L2 was destroyed as well
- (T1) B has finished broadcasting the event to L1 and destroyed the last shared_ptr
- (T1) this triggered the destructor, which called into B->RemoveListener()
- (T1) all pointers in the m_listeners list were now stale, so RemoveListener emptied the list
- (T1) Eventually control returned to the ListenerIterator() for doing broadcasting, which was
  still in the middle of iterating through the list
- (T1) Only now, it was holding onto a dangling iterator. BOOM.

I fix this issue by making sure nothing can interfere with the
iterate-and-remove-expired-pointers loop, by moving this logic into a single function, which
first locks (or clears) the whole list and then returns the list of valid and locked Listeners
for further processing. Instead of std::list I use an llvm::SmallVector which should hopefully
offset the fact that we create a copy of the list for the common case where we have only a few
listeners (no heap allocations).

A slight difference in behaviour is that now RemoveListener does not remove an element from the
list -- it only sets it's mask to 0, which means it will be removed during the next iteration of
GetListeners(). This is purely an implementation detail and it should not be externally
noticable.

I was not able to reproduce this bug reliably without inserting sleep statements into the code,
so I do not add a test for it. Instead, I add some unit tests for the functions that I do modify.

Reviewers: clayborg, jingham

Subscribers: tberghammer, lldb-commits

Differential Revision: https://reviews.llvm.org/D23406

llvm-svn: 278664
lldb/include/lldb/Core/Broadcaster.h
lldb/source/Core/Broadcaster.cpp
lldb/unittests/Core/BroadcasterTest.cpp [new file with mode: 0644]
lldb/unittests/Core/CMakeLists.txt