Wait for threads to finish unregistering
There was a race where Thread::join would return before the thread was
unregistered. This caused a problem with Daemons.stop since the thread
list could get deleted before the daemon thread was removed from list_.
This caused occasional "Request to unregister unattached thread"
errors and warnings.
The fix is to wait until threads finish registering before destroying
the thread list. The only threads which can be unregistering at this
point are the daemons we stopped earlier during the runtime shutdown
process. The issue is that thread join finishes before we remove the
thread from the thread list.
Also some cleanup.
Bug: 18713034
Change-Id: I8921122fe8462643a6b814b5f00632481e3831fb
diff --git a/runtime/base/mutex.cc b/runtime/base/mutex.cc
index 6e00cc7..13dcb8c 100644
--- a/runtime/base/mutex.cc
+++ b/runtime/base/mutex.cc
@@ -57,6 +57,7 @@
Mutex* Locks::reference_queue_weak_references_lock_ = nullptr;
Mutex* Locks::runtime_shutdown_lock_ = nullptr;
Mutex* Locks::thread_list_lock_ = nullptr;
+ConditionVariable* Locks::thread_exit_cond_ = nullptr;
Mutex* Locks::thread_suspend_count_lock_ = nullptr;
Mutex* Locks::trace_lock_ = nullptr;
Mutex* Locks::unexpected_signal_lock_ = nullptr;
@@ -1063,8 +1064,13 @@
logging_lock_ = new Mutex("logging lock", current_lock_level, true);
#undef UPDATE_CURRENT_LOCK_LEVEL
+
+ InitConditions();
}
}
+void Locks::InitConditions() {
+ thread_exit_cond_ = new ConditionVariable("thread exit condition variable", *thread_list_lock_);
+}
} // namespace art
diff --git a/runtime/base/mutex.h b/runtime/base/mutex.h
index 45d2347..3b052c0 100644
--- a/runtime/base/mutex.h
+++ b/runtime/base/mutex.h
@@ -487,7 +487,7 @@
class Locks {
public:
static void Init();
-
+ static void InitConditions() NO_THREAD_SAFETY_ANALYSIS; // Condition variables.
// Guards allocation entrypoint instrumenting.
static Mutex* instrument_entrypoints_lock_;
@@ -575,6 +575,9 @@
// attaching and detaching.
static Mutex* thread_list_lock_ ACQUIRED_AFTER(deoptimization_lock_);
+ // Signaled when threads terminate. Used to determine when all non-daemons have terminated.
+ static ConditionVariable* thread_exit_cond_ GUARDED_BY(Locks::thread_list_lock_);
+
// Guards maintaining loading library data structures.
static Mutex* jni_libraries_lock_ ACQUIRED_AFTER(thread_list_lock_);
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index 83c5ffb..31ccbcc 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -51,8 +51,7 @@
static constexpr useconds_t kThreadSuspendMaxSleepUs = 5000;
ThreadList::ThreadList()
- : suspend_all_count_(0), debug_suspend_all_count_(0),
- thread_exit_cond_("thread exit condition variable", *Locks::thread_list_lock_),
+ : suspend_all_count_(0), debug_suspend_all_count_(0), unregistering_count_(0),
suspend_all_historam_("suspend all histogram", 16, 64) {
CHECK(Monitor::IsValidLockWord(LockWord::FromThinLockId(kMaxThreadId, 1)));
}
@@ -70,7 +69,6 @@
if (contains) {
Runtime::Current()->DetachCurrentThread();
}
-
WaitForOtherNonDaemonThreadsToExit();
// TODO: there's an unaddressed race here where a thread may attach during shutdown, see
// Thread::Init.
@@ -1002,27 +1000,32 @@
void ThreadList::WaitForOtherNonDaemonThreadsToExit() {
Thread* self = Thread::Current();
Locks::mutator_lock_->AssertNotHeld(self);
- bool all_threads_are_daemons;
- do {
+ while (true) {
{
// No more threads can be born after we start to shutdown.
MutexLock mu(self, *Locks::runtime_shutdown_lock_);
CHECK(Runtime::Current()->IsShuttingDownLocked());
CHECK_EQ(Runtime::Current()->NumberOfThreadsBeingBorn(), 0U);
}
- all_threads_are_daemons = true;
MutexLock mu(self, *Locks::thread_list_lock_);
- for (const auto& thread : list_) {
- if (thread != self && !thread->IsDaemon()) {
- all_threads_are_daemons = false;
- break;
+ // Also wait for any threads that are unregistering to finish. This is required so that no
+ // threads access the thread list after it is deleted. TODO: This may not work for user daemon
+ // threads since they could unregister at the wrong time.
+ bool done = unregistering_count_ == 0;
+ if (done) {
+ for (const auto& thread : list_) {
+ if (thread != self && !thread->IsDaemon()) {
+ done = false;
+ break;
+ }
}
}
- if (!all_threads_are_daemons) {
- // Wait for another thread to exit before re-checking.
- thread_exit_cond_.Wait(self);
+ if (done) {
+ break;
}
- } while (!all_threads_are_daemons);
+ // Wait for another thread to exit before re-checking.
+ Locks::thread_exit_cond_->Wait(self);
+ }
}
void ThreadList::SuspendAllDaemonThreads() {
@@ -1092,42 +1095,45 @@
VLOG(threads) << "ThreadList::Unregister() " << *self;
+ {
+ MutexLock mu(self, *Locks::thread_list_lock_);
+ ++unregistering_count_;
+ }
+
// Any time-consuming destruction, plus anything that can call back into managed code or
- // suspend and so on, must happen at this point, and not in ~Thread.
+ // suspend and so on, must happen at this point, and not in ~Thread. The self->Destroy is what
+ // causes the threads to join. It is important to do this after incrementing unregistering_count_
+ // since we want the runtime to wait for the daemon threads to exit before deleting the thread
+ // list.
self->Destroy();
// If tracing, remember thread id and name before thread exits.
Trace::StoreExitingThreadInfo(self);
uint32_t thin_lock_id = self->GetThreadId();
- while (self != nullptr) {
+ while (true) {
// Remove and delete the Thread* while holding the thread_list_lock_ and
// thread_suspend_count_lock_ so that the unregistering thread cannot be suspended.
// Note: deliberately not using MutexLock that could hold a stale self pointer.
- Locks::thread_list_lock_->ExclusiveLock(self);
- bool removed = true;
+ MutexLock mu(self, *Locks::thread_list_lock_);
if (!Contains(self)) {
std::string thread_name;
self->GetThreadName(thread_name);
std::ostringstream os;
DumpNativeStack(os, GetTid(), " native: ", nullptr);
LOG(ERROR) << "Request to unregister unattached thread " << thread_name << "\n" << os.str();
+ break;
} else {
- Locks::thread_suspend_count_lock_->ExclusiveLock(self);
+ MutexLock mu2(self, *Locks::thread_suspend_count_lock_);
if (!self->IsSuspended()) {
list_.remove(self);
- } else {
- // We failed to remove the thread due to a suspend request, loop and try again.
- removed = false;
+ break;
}
- Locks::thread_suspend_count_lock_->ExclusiveUnlock(self);
}
- Locks::thread_list_lock_->ExclusiveUnlock(self);
- if (removed) {
- delete self;
- self = nullptr;
- }
+ // We failed to remove the thread due to a suspend request, loop and try again.
}
+ delete self;
+
// Release the thread ID after the thread is finished and deleted to avoid cases where we can
// temporarily have multiple threads with the same thread id. When this occurs, it causes
// problems in FindThreadByThreadId / SuspendThreadByThreadId.
@@ -1138,8 +1144,9 @@
CHECK_PTHREAD_CALL(pthread_setspecific, (Thread::pthread_key_self_, NULL), "detach self");
// Signal that a thread just detached.
- MutexLock mu(NULL, *Locks::thread_list_lock_);
- thread_exit_cond_.Signal(NULL);
+ MutexLock mu(nullptr, *Locks::thread_list_lock_);
+ --unregistering_count_;
+ Locks::thread_exit_cond_->Broadcast(nullptr);
}
void ThreadList::ForEach(void (*callback)(Thread*, void*), void* context) {
diff --git a/runtime/thread_list.h b/runtime/thread_list.h
index d18315a..de0dd79 100644
--- a/runtime/thread_list.h
+++ b/runtime/thread_list.h
@@ -177,8 +177,8 @@
int suspend_all_count_ GUARDED_BY(Locks::thread_suspend_count_lock_);
int debug_suspend_all_count_ GUARDED_BY(Locks::thread_suspend_count_lock_);
- // Signaled when threads terminate. Used to determine when all non-daemons have terminated.
- ConditionVariable thread_exit_cond_ GUARDED_BY(Locks::thread_list_lock_);
+ // Number of threads unregistering, ~ThreadList blocks until this hits 0.
+ int unregistering_count_ GUARDED_BY(Locks::thread_list_lock_);
// Thread suspend time histogram. Only modified when all the threads are suspended, so guarding
// by mutator lock ensures no thread can read when another thread is modifying it.
