Added UScopeMutex::lockTry() support (#1777)

* Added UScopeMutex::lockTry() support

* lockTry should not be callable on temporary lock

* fixing test coverage

CMakeLists.txt

@@ -22,7 +22,7 @@ SET(CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake_modules")
 #######################
 SET(RTABMAP_MAJOR_VERSION 0)
 SET(RTABMAP_MINOR_VERSION 23)
-SET(RTABMAP_PATCH_VERSION 12)
+SET(RTABMAP_PATCH_VERSION 13)
 SET(RTABMAP_VERSION
   ${RTABMAP_MAJOR_VERSION}.${RTABMAP_MINOR_VERSION}.${RTABMAP_PATCH_VERSION})
 

package.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0"?>
 <package format="2">
   <name>rtabmap</name>
-  <version>0.23.12</version>
+  <version>0.23.13</version>
   <description>RTAB-Map's standalone library. RTAB-Map is a RGB-D SLAM approach with real-time constraints.</description>
   <maintainer email="[email protected]">Mathieu Labbe</maintainer>
   <author>Mathieu Labbe</author>

utilite/include/rtabmap/utilite/UMutex.h

@@ -159,28 +159,131 @@ public:
  *
  * @endcode
  *
+ * The lock can also be deferred, for example to only try locking it. The destructor
+ * then unlocks the mutex only if this object locked it:
+ * @code
+ * void callback()
+ * {
+ *    UScopeMutex sm(m, false); // not locked yet
+ *    if(sm.lockTry() == 0)
+ *    {
+ *       if(cond1)
+ *       {
+ *          return; // automatically unlock the mutex m
+ *       }
+ *       ...
+ *    }
+ *    // the mutex m is unlocked only if lockTry() succeeded
+ * }
+ * @endcode
+ *
+ * The object must be named: a temporary would be destroyed, and the mutex unlocked,
+ * at the end of the expression. lock(), lockTry() and unlock() can only be called on
+ * a named object, so that `if(UScopeMutex(m, false).lockTry() == 0)` doesn't compile.
+ * In C++17, the object can be scoped to an if statement instead:
+ * @code
+ * if(UScopeMutex sm(m, false); sm.lockTry() == 0)
+ * {
+ *    // locked here
+ * } // unlocked here, only if lockTry() succeeded
+ * @endcode
+ *
  * @see UMutex
  */
 class UScopeMutex
 {
 public:
-	UScopeMutex(const UMutex & mutex) :
+	/**
-		mutex_(mutex)
+	 * @param mutex the mutex to lock.
+	 * @param lockNow if true (default), the mutex is locked here. If false, it is not
+	 *        locked until lock() or lockTry() is called.
+	 */
+	UScopeMutex(const UMutex & mutex, bool lockNow = true) :
+		mutex_(mutex),
+		locked_(false)
 	{
-		mutex_.lock();
+		if(lockNow)
+		{
+			lock();
+		}
 	}
 	// backward compatibility
 	UScopeMutex(UMutex * mutex) :
-		mutex_(*mutex)
+		mutex_(*mutex),
+		locked_(false)
 	{
-		mutex_.lock();
+		lock();
 	}
+	/**
+	 * Unlock the mutex, only if this object locked it.
+	 */
 	~UScopeMutex()
 	{
-		mutex_.unlock();
+		unlock();
 	}
+
+	/**
+	 * Lock the mutex, if this object doesn't hold it already.
+	 * @return 0 on success, an error code otherwise.
+	 */
+	int lock() &
+	{
+		if(locked_)
+		{
+			return 0;
+		}
+		int r = mutex_.lock();
+		locked_ = r == 0;
+		return r;
+	}
+
+#if !defined(_WIN32) || (_WIN32_WINNT >= 0x0400)
+	/**
+	 * Try locking the mutex, if this object doesn't hold it already.
+	 * @return 0 if the mutex is held by this object, EBUSY (or another
+	 *         error code) otherwise.
+	 */
+	int lockTry() &
+	{
+		if(locked_)
+		{
+			return 0;
+		}
+		int r = mutex_.lockTry();
+		locked_ = r == 0;
+		return r;
+	}
+#endif
+
+	/**
+	 * Unlock the mutex before this object goes out of scope, only if this object locked it.
+	 * @return 0 on success (or if this object didn't hold the mutex), an error code otherwise.
+	 */
+	int unlock() &
+	{
+		if(!locked_)
+		{
+			return 0;
+		}
+		locked_ = false;
+		return mutex_.unlock();
+	}
+
+	/**
+	 * @return true if this object currently holds the mutex.
+	 */
+	bool isLocked() const
+	{
+		return locked_;
+	}
+
+private:
+	UScopeMutex(const UScopeMutex &);
+	void operator=(const UScopeMutex &);
+
 private:
 	const UMutex & mutex_;
+	bool locked_;
 };
 
 #endif // UMUTEX_H

utilite/test/test_umutex.cpp

@@ -3,6 +3,8 @@
 #include <thread>
 #include <chrono>
 #include <atomic>
+#include <type_traits>
+#include <utility>
 #include <vector>
 
 TEST(UMutexTest, Constructor)
@@ -149,6 +151,144 @@ TEST(UMutexTest, UScopeMutexWithPointer)
     t.join();
 }
 
+// lock(), lockTry() and unlock() can only be called on a named UScopeMutex: a temporary
+// would unlock the mutex at the end of the expression, before the code it should protect.
+template<typename T, typename = void>
+struct CanLockTry : std::false_type {};
+template<typename T>
+struct CanLockTry<T, decltype(void(std::declval<T>().lockTry()))> : std::true_type {};
+static_assert(CanLockTry<UScopeMutex &>::value, "lockTry() must be callable on a named UScopeMutex");
+static_assert(!CanLockTry<UScopeMutex>::value, "lockTry() must not be callable on a temporary UScopeMutex");
+
+TEST(UMutexTest, UScopeMutexDeferredIsNotLocked)
+{
+    UMutex mutex;
+    {
+        UScopeMutex scopeMutex(mutex, false);
+        EXPECT_FALSE(scopeMutex.isLocked());
+
+        std::thread t([&mutex]() {
+            EXPECT_EQ(mutex.lockTry(), 0); // Not locked by the scope mutex
+            mutex.unlock();
+        });
+        t.join();
+    }
+    // The destructor must not unlock a mutex the scope mutex didn't lock
+    mutex.lock();
+    std::thread t([&mutex]() {
+        EXPECT_NE(mutex.lockTry(), 0); // Still locked by this thread
+    });
+    t.join();
+    mutex.unlock();
+}
+
+TEST(UMutexTest, UScopeMutexDeferredLock)
+{
+    UMutex mutex;
+    {
+        UScopeMutex scopeMutex(mutex, false);
+        EXPECT_EQ(scopeMutex.lock(), 0);
+        EXPECT_TRUE(scopeMutex.isLocked());
+
+        std::thread t([&mutex]() {
+            EXPECT_NE(mutex.lockTry(), 0); // Should fail
+        });
+        t.join();
+    }
+    std::thread t([&mutex]() {
+        EXPECT_EQ(mutex.lockTry(), 0); // Unlocked by the destructor
+        mutex.unlock();
+    });
+    t.join();
+}
+
+TEST(UMutexTest, UScopeMutexLockWhenHeld)
+{
+    UMutex mutex;
+    {
+        UScopeMutex scopeMutex(mutex); // locked by the constructor
+        EXPECT_EQ(scopeMutex.lock(), 0); // Already held: not locked a second time
+        EXPECT_TRUE(scopeMutex.isLocked());
+    }
+    std::thread t([&mutex]() {
+        EXPECT_EQ(mutex.lockTry(), 0); // Unlocked once by the destructor, and free
+        mutex.unlock();
+    });
+    t.join();
+}
+
+TEST(UMutexTest, UScopeMutexLockTrySucceeds)
+{
+    UMutex mutex;
+    {
+        UScopeMutex scopeMutex(mutex, false);
+        EXPECT_EQ(scopeMutex.lockTry(), 0);
+        EXPECT_TRUE(scopeMutex.isLocked());
+        EXPECT_EQ(scopeMutex.lockTry(), 0); // Already held: not locked a second time
+    }
+    std::thread t([&mutex]() {
+        EXPECT_EQ(mutex.lockTry(), 0); // Unlocked once by the destructor, and free
+        mutex.unlock();
+    });
+    t.join();
+}
+
+TEST(UMutexTest, UScopeMutexLockTryFails)
+{
+    UMutex mutex;
+    std::atomic<bool> locked(false);
+    std::atomic<bool> release(false);
+    std::thread owner([&]() {
+        mutex.lock();
+        locked = true;
+        while(!release) { std::this_thread::yield(); }
+        mutex.unlock();
+    });
+    while(!locked) { std::this_thread::yield(); }
+
+    {
+        UScopeMutex scopeMutex(mutex, false);
+        EXPECT_NE(scopeMutex.lockTry(), 0); // Held by the other thread
+        EXPECT_FALSE(scopeMutex.isLocked());
+    }
+    // The destructor didn't unlock the other thread's lock
+    std::thread t([&mutex]() {
+        EXPECT_NE(mutex.lockTry(), 0);
+    });
+    t.join();
+
+    release = true;
+    owner.join();
+    EXPECT_EQ(mutex.lockTry(), 0);
+    mutex.unlock();
+}
+
+TEST(UMutexTest, UScopeMutexEarlyUnlock)
+{
+    UMutex mutex;
+    {
+        UScopeMutex scopeMutex(mutex);
+        EXPECT_TRUE(scopeMutex.isLocked());
+        EXPECT_EQ(scopeMutex.unlock(), 0);
+        EXPECT_FALSE(scopeMutex.isLocked());
+        EXPECT_EQ(scopeMutex.unlock(), 0); // Nothing to unlock anymore
+
+        std::thread t([&mutex]() {
+            EXPECT_EQ(mutex.lockTry(), 0); // Released before the end of the scope
+            mutex.unlock();
+        });
+        t.join();
+
+        mutex.lock(); // Locked by this thread, not by the scope mutex
+    }
+    // The destructor must not unlock it
+    std::thread t([&mutex]() {
+        EXPECT_NE(mutex.lockTry(), 0);
+    });
+    t.join();
+    mutex.unlock();
+}
+
 TEST(UMutexTest, MultipleMutexes)
 {
     UMutex mutex1;