Revert "Notify waiters when releasing the monitor"
This reverts commit 1ebb52ca700b6d9f9c27c3ee3e688ed17a43d358.
Reason for revert: Break ART run-test ThreadStress by failing this
assertion:
dalvikvm32 E 11-06 09:43:27 27100 27851 mutex-inl.h:134] Lock level violation: holding "a thread wait mutex" (level ThreadWaitLock - 9) while locking "a monitor lock" (level MonitorLock - 54)
dalvikvm32 F 11-06 09:43:27 27100 27851 mutex-inl.h:145] Check failed: !bad_mutexes_held
Bug: 117842465
Change-Id: I888201bf5c252c8366618d9169a37e4a4cc29734
diff --git a/runtime/monitor.cc b/runtime/monitor.cc
index 0353efd..0f0a378 100644
--- a/runtime/monitor.cc
+++ b/runtime/monitor.cc
@@ -97,7 +97,6 @@
lock_count_(0),
obj_(GcRoot<mirror::Object>(obj)),
wait_set_(nullptr),
- wake_set_(nullptr),
hash_code_(hash_code),
locking_method_(nullptr),
locking_dex_pc_(0),
@@ -121,7 +120,6 @@
lock_count_(0),
obj_(GcRoot<mirror::Object>(obj)),
wait_set_(nullptr),
- wake_set_(nullptr),
hash_code_(hash_code),
locking_method_(nullptr),
locking_dex_pc_(0),
@@ -228,8 +226,7 @@
}
void Monitor::AppendToWaitSet(Thread* thread) {
- // Not checking that the owner is equal to this thread, since we've released
- // the monitor by the time this method is called.
+ DCHECK(owner_ == Thread::Current());
DCHECK(thread != nullptr);
DCHECK(thread->GetWaitNext() == nullptr) << thread->GetWaitNext();
if (wait_set_ == nullptr) {
@@ -248,29 +245,24 @@
void Monitor::RemoveFromWaitSet(Thread *thread) {
DCHECK(owner_ == Thread::Current());
DCHECK(thread != nullptr);
- auto remove = [&](Thread*& set){
- if (set != nullptr) {
- if (set == thread) {
- set = thread->GetWaitNext();
- thread->SetWaitNext(nullptr);
- return true;
- }
- Thread* t = set;
- while (t->GetWaitNext() != nullptr) {
- if (t->GetWaitNext() == thread) {
- t->SetWaitNext(thread->GetWaitNext());
- thread->SetWaitNext(nullptr);
- return true;
- }
- t = t->GetWaitNext();
- }
- }
- return false;
- };
- if (remove(wait_set_)) {
+ if (wait_set_ == nullptr) {
return;
}
- remove(wake_set_);
+ if (wait_set_ == thread) {
+ wait_set_ = thread->GetWaitNext();
+ thread->SetWaitNext(nullptr);
+ return;
+ }
+
+ Thread* t = wait_set_;
+ while (t->GetWaitNext() != nullptr) {
+ if (t->GetWaitNext() == thread) {
+ t->SetWaitNext(thread->GetWaitNext());
+ thread->SetWaitNext(nullptr);
+ return;
+ }
+ t = t->GetWaitNext();
+ }
}
void Monitor::SetObject(mirror::Object* object) {
@@ -707,102 +699,33 @@
bool Monitor::Unlock(Thread* self) {
DCHECK(self != nullptr);
uint32_t owner_thread_id = 0u;
- DCHECK(!monitor_lock_.IsExclusiveHeld(self));
- monitor_lock_.Lock(self);
- Thread* owner = owner_;
- if (owner != nullptr) {
- owner_thread_id = owner->GetThreadId();
- }
- if (owner == self) {
- // We own the monitor, so nobody else can be in here.
- AtraceMonitorUnlock();
- if (lock_count_ == 0) {
- owner_ = nullptr;
- locking_method_ = nullptr;
- locking_dex_pc_ = 0;
- SignalContendersAndReleaseMonitorLock(self);
- return true;
- } else {
- --lock_count_;
- monitor_lock_.Unlock(self);
+ {
+ MutexLock mu(self, monitor_lock_);
+ Thread* owner = owner_;
+ if (owner != nullptr) {
+ owner_thread_id = owner->GetThreadId();
+ }
+ if (owner == self) {
+ // We own the monitor, so nobody else can be in here.
+ AtraceMonitorUnlock();
+ if (lock_count_ == 0) {
+ owner_ = nullptr;
+ locking_method_ = nullptr;
+ locking_dex_pc_ = 0;
+ // Wake a contender.
+ monitor_contenders_.Signal(self);
+ } else {
+ --lock_count_;
+ }
return true;
}
}
// We don't own this, so we're not allowed to unlock it.
// The JNI spec says that we should throw IllegalMonitorStateException in this case.
FailedUnlock(GetObject(), self->GetThreadId(), owner_thread_id, this);
- monitor_lock_.Unlock(self);
return false;
}
-void Monitor::SignalContendersAndReleaseMonitorLock(Thread* self) {
- // We want to signal one thread to wake up, to acquire the monitor that
- // we are releasing. This could either be a Thread waiting on its own
- // ConditionVariable, or a thread waiting on monitor_contenders_.
- while (true) {
- Thread* thread = wake_set_;
- if (thread == nullptr) {
- break;
- } else if (thread == self) {
- // In the case of wait(), this will be invoked with self's GetWaitMutex held.
- // On a second run through this while loop, we will have released and reacquired
- // monitor_lock_, so it is possible that self has moved into wake_set. Since we
- // don't want to signal ourselves before waiting or recursively acquire GetWaitMutex,
- // skip ourself if we encounter it while traversing the wake set.
- thread = self->GetWaitNext();
- if (thread == nullptr) {
- break;
- }
- self->SetWaitNext(thread->GetWaitNext());
- } else {
- wake_set_ = thread->GetWaitNext();
- }
- thread->SetWaitNext(nullptr);
-
- // Release the lock, so that a potentially awakened thread will not
- // immediately contend on it.
- monitor_lock_.Unlock(self);
- // Check to see if the thread is still waiting.
- {
- // In the case of wait(), we'll be acquiring another thread's GetWaitMutex with
- // self's GetWaitMutex held. This does not risk deadlock, because we only acquire this lock
- // for threads in the wake_set_. A thread can only enter wake_set_ from Notify or NotifyAll,
- // and those acquire each thread's GetWaitMutex before moving them. Thus, the threads whose
- // wait mutexes we acquire here must have already been released from wait(), so there is no
- // risk of the following lock ordering leading to deadlock:
- // Thread 1 waits
- // Thread 2 waits
- // While threads 1 and 2 have released both the monitor and the monitor_lock_, thread 3 calls
- // notify() to wake them both up.
- // Thread 1 enters this block, and attempts to acquire Thread 2's GetWaitMutex to wake it
- // Thread 2 enters this block, and attempts to acquire Thread 1's GetWaitMutex to wake it
- //
- // Thanks to the lock checking in Notify and NotifyAll, no thread is observable in wake_set_
- // unless that thread has actually started waiting (and therefore will not subsequently
- // acquire another thread's GetWaitMutex while holding its own).
- MutexLock wait_mu(self, *thread->GetWaitMutex());
- if (thread->GetWaitMonitor() != nullptr) {
- thread->GetWaitConditionVariable()->Signal(self);
- return;
- }
- }
- // Reacquire the lock for the next iteration
- monitor_lock_.Lock(self);
- // We had to reacquire the lock so that we can wake a contender, or look
- // for another notified waiter thread, but if someone else has already acquired our
- // monitor, there's no need to wake anybody else as they'll just contend
- // with the current owner.
- if (owner_ != nullptr) {
- monitor_lock_.Unlock(self);
- return;
- }
- }
- // If we didn't wake any threads that were originally waiting on us,
- // wake a contender.
- monitor_contenders_.Signal(self);
- monitor_lock_.Unlock(self);
-}
-
void Monitor::Wait(Thread* self, int64_t ms, int32_t ns,
bool interruptShouldThrow, ThreadState why) {
DCHECK(self != nullptr);
@@ -832,9 +755,17 @@
}
/*
- * Release our hold - we need to let it go even if we're a few levels
+ * Add ourselves to the set of threads waiting on this monitor, and
+ * release our hold. We need to let it go even if we're a few levels
* deep in a recursive lock, and we need to restore that later.
+ *
+ * We append to the wait set ahead of clearing the count and owner
+ * fields so the subroutine can check that the calling thread owns
+ * the monitor. Aside from that, the order of member updates is
+ * not order sensitive as we hold the pthread mutex.
*/
+ AppendToWaitSet(self);
+ ++num_waiters_;
int prev_lock_count = lock_count_;
lock_count_ = 0;
owner_ = nullptr;
@@ -859,17 +790,6 @@
// Pseudo-atomically wait on self's wait_cond_ and release the monitor lock.
MutexLock mu(self, *self->GetWaitMutex());
- /*
- * Add ourselves to the set of threads waiting on this monitor.
- * It's important that we are only added to the wait set after
- * acquiring our GetWaitMutex, so that calls to Notify() that occur after we
- * have released monitor_lock_ will not move us from wait_set_ to wake_set_
- * until we've signalled contenders on this monitor.
- */
- AppendToWaitSet(self);
- ++num_waiters_;
-
-
// Set wait_monitor_ to the monitor object we will be waiting on. When wait_monitor_ is
// non-null a notifying or interrupting thread must signal the thread's wait_cond_ to wake it
// up.
@@ -877,7 +797,8 @@
self->SetWaitMonitor(this);
// Release the monitor lock.
- SignalContendersAndReleaseMonitorLock(self);
+ monitor_contenders_.Signal(self);
+ monitor_lock_.Unlock(self);
// Handle the case where the thread was interrupted before we called wait().
if (self->IsInterrupted()) {
@@ -953,16 +874,18 @@
ThrowIllegalMonitorStateExceptionF("object not locked by thread before notify()");
return;
}
- // Move one thread from waiters to wake set
- Thread* to_move = wait_set_;
- if (to_move != nullptr) {
- // Acquiring the thread's wait mutex before moving it prevents us from moving a thread that
- // has released monitor_lock_ in wait() but not yet tried to wake an entry in wake_set_. See
- // comments in SignalContendersAndReleaseMonitorLock.
- MutexLock wait(self, *to_move->GetWaitMutex());
- wait_set_ = to_move->GetWaitNext();
- to_move->SetWaitNext(wake_set_);
- wake_set_ = to_move;
+ // Signal the first waiting thread in the wait set.
+ while (wait_set_ != nullptr) {
+ Thread* thread = wait_set_;
+ wait_set_ = thread->GetWaitNext();
+ thread->SetWaitNext(nullptr);
+
+ // Check to see if the thread is still waiting.
+ MutexLock wait_mu(self, *thread->GetWaitMutex());
+ if (thread->GetWaitMonitor() != nullptr) {
+ thread->GetWaitConditionVariable()->Signal(self);
+ return;
+ }
}
}
@@ -974,17 +897,12 @@
ThrowIllegalMonitorStateExceptionF("object not locked by thread before notifyAll()");
return;
}
-
- // Move all threads from waiters to wake set
+ // Signal all threads in the wait set.
while (wait_set_ != nullptr) {
- Thread* to_move = wait_set_;
- // Acquiring the thread's wait mutex before moving it prevents us from moving a thread that
- // has released monitor_lock_ in wait() but not yet tried to wake an entry in wake_set_. See
- // comments in SignalContendersAndReleaseMonitorLock.
- MutexLock wait(self, *to_move->GetWaitMutex());
- wait_set_ = to_move->GetWaitNext();
- to_move->SetWaitNext(wake_set_);
- wake_set_ = to_move;
+ Thread* thread = wait_set_;
+ wait_set_ = thread->GetWaitNext();
+ thread->SetWaitNext(nullptr);
+ thread->Notify();
}
}