Merge "MediaMetrics:LockWrap: Use recursive mutex."
diff --git a/services/mediametrics/Wrap.h b/services/mediametrics/Wrap.h
index 99963fb..3584e08 100644
--- a/services/mediametrics/Wrap.h
+++ b/services/mediametrics/Wrap.h
@@ -150,11 +150,17 @@
*/
class LockedPointer {
friend LockWrap;
- LockedPointer(T *t, std::mutex *lock)
- : mT(t), mLock(*lock) {}
+ LockedPointer(T *t, std::recursive_mutex *lock, std::atomic<size_t> *recursionDepth)
+ : mT(t), mLock(*lock), mRecursionDepth(recursionDepth) { ++*mRecursionDepth; }
+
T* const mT;
- std::lock_guard<std::mutex> mLock;
+ std::lock_guard<std::recursive_mutex> mLock;
+ std::atomic<size_t>* mRecursionDepth;
public:
+ ~LockedPointer() {
+ --*mRecursionDepth; // Used for testing, we do not check underflow.
+ }
+
const T* operator->() const {
return mT;
}
@@ -163,27 +169,36 @@
}
};
- mutable std::mutex mLock;
+ // We must use a recursive mutex because the end of the full expression may
+ // involve another reference to T->.
+ //
+ // A recursive mutex allows the same thread to recursively acquire,
+ // but different thread would block.
+ //
+ // Example which fails with a normal mutex:
+ //
+ // android::mediametrics::LockWrap<std::vector<int>> v{std::initializer_list<int>{1, 2}};
+ // const int sum = v->operator[](0) + v->operator[](1);
+ //
+ mutable std::recursive_mutex mLock;
mutable T mT;
+ mutable std::atomic<size_t> mRecursionDepth{}; // Used for testing.
public:
template <typename... Args>
explicit LockWrap(Args&&... args) : mT(std::forward<Args>(args)...) {}
const LockedPointer operator->() const {
- return LockedPointer(&mT, &mLock);
+ return LockedPointer(&mT, &mLock, &mRecursionDepth);
}
LockedPointer operator->() {
- return LockedPointer(&mT, &mLock);
+ return LockedPointer(&mT, &mLock, &mRecursionDepth);
}
+ // Returns the lock depth of the recursive mutex.
// @TestApi
- bool isLocked() const {
- if (mLock.try_lock()) {
- mLock.unlock();
- return false; // we were able to get the lock.
- }
- return true; // we were NOT able to get the lock.
+ size_t getRecursionDepth() const {
+ return mRecursionDepth;
}
};
diff --git a/services/mediametrics/tests/mediametrics_tests.cpp b/services/mediametrics/tests/mediametrics_tests.cpp
index dea625c..27b72eb 100644
--- a/services/mediametrics/tests/mediametrics_tests.cpp
+++ b/services/mediametrics/tests/mediametrics_tests.cpp
@@ -112,6 +112,27 @@
s3->operator=("abc");
ASSERT_EQ('b', s3->operator[](1)); // s2[1] = 'b';
+ // Check that we can recursively hold lock.
+ android::mediametrics::LockWrap<std::vector<int>> v{std::initializer_list<int>{1, 2}};
+ v->push_back(3);
+ v->push_back(4);
+ ASSERT_EQ(1, v->operator[](0));
+ ASSERT_EQ(2, v->operator[](1));
+ ASSERT_EQ(3, v->operator[](2));
+ ASSERT_EQ(4, v->operator[](3));
+ // The end of the full expression here requires recursive depth of 4.
+ ASSERT_EQ(10, v->operator[](0) + v->operator[](1) + v->operator[](2) + v->operator[](3));
+
+ // Mikhail's note: a non-recursive lock implementation could be used if one obtains
+ // the LockedPointer helper object like this and directly hold the lock through RAII,
+ // though it is trickier in use.
+ //
+ // We include an example here for completeness.
+ {
+ auto l = v.operator->();
+ ASSERT_EQ(10, l->operator[](0) + l->operator[](1) + l->operator[](2) + l->operator[](3));
+ }
+
// Use Thunk to check whether we have the lock when calling a method through LockWrap.
class Thunk {
@@ -123,11 +144,13 @@
};
android::mediametrics::LockWrap<Thunk> s4([&]{
- ASSERT_EQ(true, s4.isLocked()); // we must be locked when thunk() is called.
+ ASSERT_EQ((size_t)1, s4.getRecursionDepth()); // we must be locked when thunk() is called.
});
+ ASSERT_EQ((size_t)0, s4.getRecursionDepth());
// This will fail if we are not locked during method access.
s4->thunk();
+ ASSERT_EQ((size_t)0, s4.getRecursionDepth());
}
TEST(mediametrics_tests, lock_wrap_multithread) {