Revert "Use try lock to fix class resolution race"
This reverts commit a704eda0078989a73cac111ed309aca50d2e289b.
Bug: 27417671
Bug: 30500547
Change-Id: Ieea05236b9e61c722660cd9497c9d55d13ccd010
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index f13fea0..d0dad64 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -2179,33 +2179,20 @@
}
// Wait for the class if it has not already been linked.
- size_t index = 0;
- // Maximum number of yield iterations until we start sleeping.
- static const size_t kNumYieldIterations = 1000;
- // How long each sleep is in us.
- static const size_t kSleepDurationUS = 1000; // 1 ms.
- while (!klass->IsResolved() && !klass->IsErroneous()) {
+ if (!klass->IsResolved() && !klass->IsErroneous()) {
StackHandleScope<1> hs(self);
HandleWrapper<mirror::Class> h_class(hs.NewHandleWrapper(&klass));
- {
- ObjectTryLock<mirror::Class> lock(self, h_class);
- // Can not use a monitor wait here since it may block when returning and deadlock if another
- // thread has locked klass.
- if (lock.Acquired()) {
- // Check for circular dependencies between classes, the lock is required for SetStatus.
- if (!h_class->IsResolved() && h_class->GetClinitThreadId() == self->GetTid()) {
- ThrowClassCircularityError(h_class.Get());
- mirror::Class::SetStatus(h_class, mirror::Class::kStatusError, self);
- return nullptr;
- }
- }
+ ObjectLock<mirror::Class> lock(self, h_class);
+ // Check for circular dependencies between classes.
+ if (!h_class->IsResolved() && h_class->GetClinitThreadId() == self->GetTid()) {
+ ThrowClassCircularityError(h_class.Get());
+ mirror::Class::SetStatus(h_class, mirror::Class::kStatusError, self);
+ return nullptr;
}
- if (index < kNumYieldIterations) {
- sched_yield();
- } else {
- usleep(kSleepDurationUS);
+ // Wait for the pending initialization to complete.
+ while (!h_class->IsResolved() && !h_class->IsErroneous()) {
+ lock.WaitIgnoringInterrupts();
}
- ++index;
}
if (klass->IsErroneous()) {
diff --git a/runtime/mirror/object-inl.h b/runtime/mirror/object-inl.h
index e1097fa..76a36ac 100644
--- a/runtime/mirror/object-inl.h
+++ b/runtime/mirror/object-inl.h
@@ -106,11 +106,7 @@
}
inline mirror::Object* Object::MonitorEnter(Thread* self) {
- return Monitor::MonitorEnter(self, this, /*trylock*/false);
-}
-
-inline mirror::Object* Object::MonitorTryEnter(Thread* self) {
- return Monitor::MonitorEnter(self, this, /*trylock*/true);
+ return Monitor::MonitorEnter(self, this);
}
inline bool Object::MonitorExit(Thread* self) {
diff --git a/runtime/mirror/object.h b/runtime/mirror/object.h
index e174cbc..0ee46c3 100644
--- a/runtime/mirror/object.h
+++ b/runtime/mirror/object.h
@@ -140,11 +140,6 @@
SHARED_REQUIRES(Locks::mutator_lock_);
uint32_t GetLockOwnerThreadId();
- // Try to enter the monitor, returns non null if we succeeded.
- mirror::Object* MonitorTryEnter(Thread* self)
- EXCLUSIVE_LOCK_FUNCTION()
- REQUIRES(!Roles::uninterruptible_)
- SHARED_REQUIRES(Locks::mutator_lock_);
mirror::Object* MonitorEnter(Thread* self)
EXCLUSIVE_LOCK_FUNCTION()
REQUIRES(!Roles::uninterruptible_)
diff --git a/runtime/monitor.cc b/runtime/monitor.cc
index bf9f931..396c946 100644
--- a/runtime/monitor.cc
+++ b/runtime/monitor.cc
@@ -314,34 +314,21 @@
return oss.str();
}
-bool Monitor::TryLockLocked(Thread* self) {
- if (owner_ == nullptr) { // Unowned.
- owner_ = self;
- CHECK_EQ(lock_count_, 0);
- // When debugging, save the current monitor holder for future
- // acquisition failures to use in sampled logging.
- if (lock_profiling_threshold_ != 0) {
- locking_method_ = self->GetCurrentMethod(&locking_dex_pc_);
- }
- } else if (owner_ == self) { // Recursive.
- lock_count_++;
- } else {
- return false;
- }
- AtraceMonitorLock(self, GetObject(), false /* is_wait */);
- return true;
-}
-
-bool Monitor::TryLock(Thread* self) {
- MutexLock mu(self, monitor_lock_);
- return TryLockLocked(self);
-}
-
void Monitor::Lock(Thread* self) {
MutexLock mu(self, monitor_lock_);
while (true) {
- if (TryLockLocked(self)) {
- return;
+ if (owner_ == nullptr) { // Unowned.
+ owner_ = self;
+ CHECK_EQ(lock_count_, 0);
+ // When debugging, save the current monitor holder for future
+ // acquisition failures to use in sampled logging.
+ if (lock_profiling_threshold_ != 0) {
+ locking_method_ = self->GetCurrentMethod(&locking_dex_pc_);
+ }
+ break;
+ } else if (owner_ == self) { // Recursive.
+ lock_count_++;
+ break;
}
// Contended.
const bool log_contention = (lock_profiling_threshold_ != 0);
@@ -443,6 +430,8 @@
monitor_lock_.Lock(self); // Reacquire locks in order.
--num_waiters_;
}
+
+ AtraceMonitorLock(self, GetObject(), false /* is_wait */);
}
static void ThrowIllegalMonitorStateExceptionF(const char* fmt, ...)
@@ -863,7 +852,7 @@
return obj;
}
-mirror::Object* Monitor::MonitorEnter(Thread* self, mirror::Object* obj, bool trylock) {
+mirror::Object* Monitor::MonitorEnter(Thread* self, mirror::Object* obj) {
DCHECK(self != nullptr);
DCHECK(obj != nullptr);
self->AssertThreadSuspensionIsAllowable();
@@ -909,9 +898,6 @@
InflateThinLocked(self, h_obj, lock_word, 0);
}
} else {
- if (trylock) {
- return nullptr;
- }
// Contention.
contention_count++;
Runtime* runtime = Runtime::Current();
@@ -930,12 +916,8 @@
}
case LockWord::kFatLocked: {
Monitor* mon = lock_word.FatLockMonitor();
- if (trylock) {
- return mon->TryLock(self) ? h_obj.Get() : nullptr;
- } else {
- mon->Lock(self);
- return h_obj.Get(); // Success!
- }
+ mon->Lock(self);
+ return h_obj.Get(); // Success!
}
case LockWord::kHashCode:
// Inflate with the existing hashcode.
diff --git a/runtime/monitor.h b/runtime/monitor.h
index 1d829e1..7b4b8f9 100644
--- a/runtime/monitor.h
+++ b/runtime/monitor.h
@@ -62,7 +62,7 @@
NO_THREAD_SAFETY_ANALYSIS; // TODO: Reading lock owner without holding lock is racy.
// NO_THREAD_SAFETY_ANALYSIS for mon->Lock.
- static mirror::Object* MonitorEnter(Thread* thread, mirror::Object* obj, bool trylock)
+ static mirror::Object* MonitorEnter(Thread* thread, mirror::Object* obj)
EXCLUSIVE_LOCK_FUNCTION(obj)
NO_THREAD_SAFETY_ANALYSIS
REQUIRES(!Roles::uninterruptible_)
@@ -193,15 +193,6 @@
!monitor_lock_)
SHARED_REQUIRES(Locks::mutator_lock_);
- // Try to lock without blocking, returns true if we acquired the lock.
- bool TryLock(Thread* self)
- REQUIRES(!monitor_lock_)
- SHARED_REQUIRES(Locks::mutator_lock_);
- // Variant for already holding the monitor lock.
- bool TryLockLocked(Thread* self)
- REQUIRES(monitor_lock_)
- SHARED_REQUIRES(Locks::mutator_lock_);
-
void Lock(Thread* self)
REQUIRES(!monitor_lock_)
SHARED_REQUIRES(Locks::mutator_lock_);
diff --git a/runtime/monitor_test.cc b/runtime/monitor_test.cc
index 48d256c..83e0c0d 100644
--- a/runtime/monitor_test.cc
+++ b/runtime/monitor_test.cc
@@ -26,7 +26,6 @@
#include "handle_scope-inl.h"
#include "mirror/class-inl.h"
#include "mirror/string-inl.h" // Strings are easiest to allocate
-#include "object_lock.h"
#include "scoped_thread_state_change.h"
#include "thread_pool.h"
@@ -375,60 +374,4 @@
"Monitor test thread pool 3");
}
-class TryLockTask : public Task {
- public:
- explicit TryLockTask(Handle<mirror::Object> obj) : obj_(obj) {}
-
- void Run(Thread* self) {
- ScopedObjectAccess soa(self);
- // Lock is held by other thread, try lock should fail.
- ObjectTryLock<mirror::Object> lock(self, obj_);
- EXPECT_FALSE(lock.Acquired());
- }
-
- void Finalize() {
- delete this;
- }
-
- private:
- Handle<mirror::Object> obj_;
-};
-
-// Test trylock in deadlock scenarios.
-TEST_F(MonitorTest, TestTryLock) {
- ScopedLogSeverity sls(LogSeverity::FATAL);
-
- Thread* const self = Thread::Current();
- ThreadPool thread_pool("the pool", 2);
- ScopedObjectAccess soa(self);
- StackHandleScope<3> hs(self);
- Handle<mirror::Object> obj1(
- hs.NewHandle<mirror::Object>(mirror::String::AllocFromModifiedUtf8(self, "hello, world!")));
- Handle<mirror::Object> obj2(
- hs.NewHandle<mirror::Object>(mirror::String::AllocFromModifiedUtf8(self, "hello, world!")));
- {
- ObjectLock<mirror::Object> lock1(self, obj1);
- ObjectLock<mirror::Object> lock2(self, obj1);
- {
- ObjectTryLock<mirror::Object> trylock(self, obj1);
- EXPECT_TRUE(trylock.Acquired());
- }
- // Test failure case.
- thread_pool.AddTask(self, new TryLockTask(obj1));
- thread_pool.StartWorkers(self);
- ScopedThreadSuspension sts(self, kSuspended);
- thread_pool.Wait(Thread::Current(), /*do_work*/false, /*may_hold_locks*/false);
- }
- // Test that the trylock actually locks the object.
- {
- ObjectTryLock<mirror::Object> trylock(self, obj1);
- EXPECT_TRUE(trylock.Acquired());
- obj1->Notify(self);
- // Since we hold the lock there should be no monitor state exeception.
- self->AssertNoPendingException();
- }
- thread_pool.StopWorkers(self);
-}
-
-
} // namespace art
diff --git a/runtime/object_lock.cc b/runtime/object_lock.cc
index b8754a4..f7accc0 100644
--- a/runtime/object_lock.cc
+++ b/runtime/object_lock.cc
@@ -47,22 +47,7 @@
obj_->NotifyAll(self_);
}
-template <typename T>
-ObjectTryLock<T>::ObjectTryLock(Thread* self, Handle<T> object) : self_(self), obj_(object) {
- CHECK(object.Get() != nullptr);
- acquired_ = obj_->MonitorTryEnter(self_) != nullptr;
-}
-
-template <typename T>
-ObjectTryLock<T>::~ObjectTryLock() {
- if (acquired_) {
- obj_->MonitorExit(self_);
- }
-}
-
template class ObjectLock<mirror::Class>;
template class ObjectLock<mirror::Object>;
-template class ObjectTryLock<mirror::Class>;
-template class ObjectTryLock<mirror::Object>;
} // namespace art
diff --git a/runtime/object_lock.h b/runtime/object_lock.h
index 7f02b37..eb7cbd8 100644
--- a/runtime/object_lock.h
+++ b/runtime/object_lock.h
@@ -45,27 +45,6 @@
DISALLOW_COPY_AND_ASSIGN(ObjectLock);
};
-template <typename T>
-class ObjectTryLock {
- public:
- ObjectTryLock(Thread* self, Handle<T> object) SHARED_REQUIRES(Locks::mutator_lock_);
-
- ~ObjectTryLock() SHARED_REQUIRES(Locks::mutator_lock_);
-
- bool Acquired() const {
- return acquired_;
- }
-
- private:
- Thread* const self_;
- Handle<T> const obj_;
- bool acquired_;
-
-
- DISALLOW_COPY_AND_ASSIGN(ObjectTryLock);
-};
-
-
} // namespace art
#endif // ART_RUNTIME_OBJECT_LOCK_H_