[Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Greg Clayton via lldb-commits]

clayborg added a comment.

HostThread::Reset() is a bad function and should not be directly used by anyone other that Join() or Detach(). Just clearing the thread ID and setting the result to zero is bad and means we will leak a thread if no one else joins it. If someone else already called Detach() on the thread, then Reset() is ok. Also calling m_private_state_thread.Cancel() is equally bad.

The real question is why is the cancel timing out?

I agree that the call to "m_private_state_thread.Reset();" should be removed from Process::RunPrivateStateThread(). But we should look at why we need to call Cancel in the first place. We should never need to cancel our own thread unless something really bad has gone on. So it seems like the real bug here is bad thread synchronization leading to issues.

I don't like the fact that we are not copying the thread anymore. We have code that sometimes needs to spin up an extra private state thread and I wouldn't want the code that changes the m_private_state_thread after backing up an older one, cause this function to call IsJoinable(), Cancel() and Join() on different objects. So it seems like the only needed fix here is to not call "m_private_state_thread.Reset();" in Process::RunPrivateStateThread(), all other changes can be removed?


Repository:
  rL LLVM

http://reviews.llvm.org/D19122