Fix issue with Thread.setName hanging after Thread.start
When suspending a thread by peer the invariant that only attached threads are
suspended must be maintained. Add a ThreadList::Contains check which requires
making this method non-static.
Add some extra thread logging.
Bug: 17302037
Change-Id: I89af9840e398c2ceadeb736c431dfe3be8f3ba06
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 6f431c3..0092160 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -2281,8 +2281,9 @@
// trying to suspend this one.
MutexLock mu(self, *Locks::thread_list_suspend_thread_lock_);
bool timed_out;
- Thread* thread = ThreadList::SuspendThreadByPeer(peer.get(), request_suspension, true,
- &timed_out);
+ ThreadList* thread_list = Runtime::Current()->GetThreadList();
+ Thread* thread = thread_list->SuspendThreadByPeer(peer.get(), request_suspension, true,
+ &timed_out);
if (thread != NULL) {
return JDWP::ERR_NONE;
} else if (timed_out) {
@@ -3170,8 +3171,8 @@
{
// Take suspend thread lock to avoid races with threads trying to suspend this one.
MutexLock mu(soa.Self(), *Locks::thread_list_suspend_thread_lock_);
- suspended_thread = ThreadList::SuspendThreadByPeer(thread_peer, true, true,
- &timed_out);
+ ThreadList* thread_list = Runtime::Current()->GetThreadList();
+ suspended_thread = thread_list->SuspendThreadByPeer(thread_peer, true, true, &timed_out);
}
CHECK_EQ(soa.Self()->TransitionFromSuspendedToRunnable(), kWaitingForDebuggerSuspension);
if (suspended_thread == nullptr) {
diff --git a/runtime/native/dalvik_system_VMStack.cc b/runtime/native/dalvik_system_VMStack.cc
index b079229..eef1c46 100644
--- a/runtime/native/dalvik_system_VMStack.cc
+++ b/runtime/native/dalvik_system_VMStack.cc
@@ -34,12 +34,13 @@
} else {
// Suspend thread to build stack trace.
soa.Self()->TransitionFromRunnableToSuspended(kNative);
+ ThreadList* thread_list = Runtime::Current()->GetThreadList();
bool timed_out;
Thread* thread;
{
// Take suspend thread lock to avoid races with threads trying to suspend this one.
MutexLock mu(soa.Self(), *Locks::thread_list_suspend_thread_lock_);
- thread = ThreadList::SuspendThreadByPeer(peer, true, false, &timed_out);
+ thread = thread_list->SuspendThreadByPeer(peer, true, false, &timed_out);
}
if (thread != nullptr) {
// Must be runnable to create returned array.
@@ -47,7 +48,7 @@
trace = thread->CreateInternalStackTrace<false>(soa);
soa.Self()->TransitionFromRunnableToSuspended(kNative);
// Restart suspended thread.
- Runtime::Current()->GetThreadList()->Resume(thread, false);
+ thread_list->Resume(thread, false);
} else {
if (timed_out) {
LOG(ERROR) << "Trying to get thread's stack failed as the thread failed to suspend within a "
diff --git a/runtime/native/java_lang_Thread.cc b/runtime/native/java_lang_Thread.cc
index 8f83f96..c0c7265 100644
--- a/runtime/native/java_lang_Thread.cc
+++ b/runtime/native/java_lang_Thread.cc
@@ -128,19 +128,20 @@
// Suspend thread to avoid it from killing itself while we set its name. We don't just hold the
// thread list lock to avoid this, as setting the thread name causes mutator to lock/unlock
// in the DDMS send code.
+ ThreadList* thread_list = Runtime::Current()->GetThreadList();
bool timed_out;
// Take suspend thread lock to avoid races with threads trying to suspend this one.
Thread* thread;
{
MutexLock mu(self, *Locks::thread_list_suspend_thread_lock_);
- thread = ThreadList::SuspendThreadByPeer(peer, true, false, &timed_out);
+ thread = thread_list->SuspendThreadByPeer(peer, true, false, &timed_out);
}
if (thread != NULL) {
{
ScopedObjectAccess soa(env);
thread->SetThreadName(name.c_str());
}
- Runtime::Current()->GetThreadList()->Resume(thread, false);
+ thread_list->Resume(thread, false);
} else if (timed_out) {
LOG(ERROR) << "Trying to set thread name to '" << name.c_str() << "' failed as the thread "
"failed to suspend within a generous timeout.";
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index 9d85ec3..fc687dc 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -406,7 +406,8 @@
void ThreadList::Resume(Thread* thread, bool for_debugger) {
Thread* self = Thread::Current();
DCHECK_NE(thread, self);
- VLOG(threads) << "Resume(" << *thread << ") starting..." << (for_debugger ? " (debugger)" : "");
+ VLOG(threads) << "Resume(" << reinterpret_cast<void*>(thread) << ") starting..."
+ << (for_debugger ? " (debugger)" : "");
{
// To check Contains.
@@ -415,18 +416,22 @@
MutexLock mu2(self, *Locks::thread_suspend_count_lock_);
DCHECK(thread->IsSuspended());
if (!Contains(thread)) {
+ // We only expect threads within the thread-list to have been suspended otherwise we can't
+ // stop such threads from delete-ing themselves.
+ LOG(ERROR) << "Resume(" << reinterpret_cast<void*>(thread)
+ << ") thread not within thread list";
return;
}
thread->ModifySuspendCount(self, -1, for_debugger);
}
{
- VLOG(threads) << "Resume(" << *thread << ") waking others";
+ VLOG(threads) << "Resume(" << reinterpret_cast<void*>(thread) << ") waking others";
MutexLock mu(self, *Locks::thread_suspend_count_lock_);
Thread::resume_cond_->Broadcast(self);
}
- VLOG(threads) << "Resume(" << *thread << ") complete";
+ VLOG(threads) << "Resume(" << reinterpret_cast<void*>(thread) << ") complete";
}
static void ThreadSuspendByPeerWarning(Thread* self, int level, const char* message, jobject peer) {
@@ -451,6 +456,7 @@
bool did_suspend_request = false;
*timed_out = false;
Thread* self = Thread::Current();
+ VLOG(threads) << "SuspendThreadByPeer starting";
while (true) {
Thread* thread;
{
@@ -462,10 +468,16 @@
ScopedObjectAccess soa(self);
MutexLock mu(self, *Locks::thread_list_lock_);
thread = Thread::FromManagedThread(soa, peer);
- if (thread == NULL) {
+ if (thread == nullptr) {
ThreadSuspendByPeerWarning(self, WARNING, "No such thread for suspend", peer);
- return NULL;
+ return nullptr;
}
+ if (!Contains(thread)) {
+ VLOG(threads) << "SuspendThreadByPeer failed for unattached thread: "
+ << reinterpret_cast<void*>(thread);
+ return nullptr;
+ }
+ VLOG(threads) << "SuspendThreadByPeer found thread: " << *thread;
{
MutexLock mu(self, *Locks::thread_suspend_count_lock_);
if (request_suspension) {
@@ -485,6 +497,7 @@
// count, or else we've waited and it has self suspended) or is the current thread, we're
// done.
if (thread->IsSuspended()) {
+ VLOG(threads) << "SuspendThreadByPeer thread suspended: " << *thread;
return thread;
}
if (total_delay_us >= kTimeoutUs) {
@@ -493,11 +506,12 @@
thread->ModifySuspendCount(soa.Self(), -1, debug_suspension);
}
*timed_out = true;
- return NULL;
+ return nullptr;
}
}
// Release locks and come out of runnable state.
}
+ VLOG(threads) << "SuspendThreadByPeer sleeping to allow thread chance to suspend";
ThreadSuspendSleep(self, &delay_us, &total_delay_us);
}
}
@@ -515,6 +529,7 @@
Thread* suspended_thread = nullptr;
Thread* self = Thread::Current();
CHECK_NE(thread_id, kInvalidThreadId);
+ VLOG(threads) << "SuspendThreadByThreadId starting";
while (true) {
{
// Note: this will transition to runnable and potentially suspend. We ensure only one thread
@@ -536,8 +551,10 @@
<< " no longer in thread list";
// There's a race in inflating a lock and the owner giving up ownership and then dying.
ThreadSuspendByThreadIdWarning(WARNING, "No such thread id for suspend", thread_id);
- return NULL;
+ return nullptr;
}
+ VLOG(threads) << "SuspendThreadByThreadId found thread: " << *thread;
+ DCHECK(Contains(thread));
{
MutexLock mu(self, *Locks::thread_suspend_count_lock_);
if (suspended_thread == nullptr) {
@@ -557,6 +574,7 @@
// count, or else we've waited and it has self suspended) or is the current thread, we're
// done.
if (thread->IsSuspended()) {
+ VLOG(threads) << "SuspendThreadByThreadId thread suspended: " << *thread;
return thread;
}
if (total_delay_us >= kTimeoutUs) {
@@ -565,11 +583,12 @@
thread->ModifySuspendCount(soa.Self(), -1, debug_suspension);
}
*timed_out = true;
- return NULL;
+ return nullptr;
}
}
// Release locks and come out of runnable state.
}
+ VLOG(threads) << "SuspendThreadByThreadId sleeping to allow thread chance to suspend";
ThreadSuspendSleep(self, &delay_us, &total_delay_us);
}
}
diff --git a/runtime/thread_list.h b/runtime/thread_list.h
index 1b67ac0..bb4f775 100644
--- a/runtime/thread_list.h
+++ b/runtime/thread_list.h
@@ -66,8 +66,8 @@
// If the thread should be suspended then value of request_suspension should be true otherwise
// the routine will wait for a previous suspend request. If the suspension times out then *timeout
// is set to true.
- static Thread* SuspendThreadByPeer(jobject peer, bool request_suspension, bool debug_suspension,
- bool* timed_out)
+ Thread* SuspendThreadByPeer(jobject peer, bool request_suspension, bool debug_suspension,
+ bool* timed_out)
EXCLUSIVE_LOCKS_REQUIRED(Locks::thread_list_suspend_thread_lock_)
LOCKS_EXCLUDED(Locks::mutator_lock_,
Locks::thread_list_lock_,
diff --git a/test/051-thread/expected.txt b/test/051-thread/expected.txt
index 7139b7f..943d1df 100644
--- a/test/051-thread/expected.txt
+++ b/test/051-thread/expected.txt
@@ -6,4 +6,7 @@
testThreadDaemons @ Thread bailing
testThreadDaemons finished
testSleepZero finished
+testSetName starting
+testSetName running
+testSetName finished
thread test done
diff --git a/test/051-thread/src/Main.java b/test/051-thread/src/Main.java
index 608b7e0..390685d 100644
--- a/test/051-thread/src/Main.java
+++ b/test/051-thread/src/Main.java
@@ -25,6 +25,7 @@
testThreadCapacity();
testThreadDaemons();
testSleepZero();
+ testSetName();
System.out.println("thread test done");
}
@@ -112,4 +113,24 @@
}
System.out.print("testSleepZero finished\n");
}
+
+ private static void testSetName() throws Exception {
+ System.out.print("testSetName starting\n");
+ Thread thread = new Thread() {
+ @Override
+ public void run() {
+ System.out.print("testSetName running\n");
+ }
+ };
+ thread.start();
+ thread.setName("HelloWorld"); // b/17302037 hang if setName called after start
+ if (!thread.getName().equals("HelloWorld")) {
+ throw new AssertionError("Unexpected thread name: " + thread.getName());
+ }
+ thread.join();
+ if (!thread.getName().equals("HelloWorld")) {
+ throw new AssertionError("Unexpected thread name after join: " + thread.getName());
+ }
+ System.out.print("testSetName finished\n");
+ }
}
