Merge "MediaMetrics:LockWrap: Use recursive mutex."

services/mediametrics/Wrap.h

@@ -150,11 +150,17 @@ class LockWrap {
       */
     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 @@ class LockWrap {
         }
     };
 
-    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;
     }
 };
 

services/mediametrics/tests/mediametrics_tests.cpp

@@ -112,6 +112,27 @@ TEST(mediametrics_tests, lock_wrap) {
   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 @@ TEST(mediametrics_tests, lock_wrap) {
   };
 
   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) {