[1704] Use the is_locked_ flag to call unlock() just once

diff --git a/src/lib/log/tests/logger_lock_test.sh.in b/src/lib/log/tests/logger_lock_test.sh.in
index a1f586598c20b289218412e2433e4fcf86c43312..0324499329b1e380f189d01bb294719a06ceaafa 100755 (executable)
--- a/src/lib/log/tests/logger_lock_test.sh.in
+++ b/src/lib/log/tests/logger_lock_test.sh.in
@@ -34,7 +34,6 @@ cat > $tempfile << .
 LOGGER_LOCK_TEST: LOCK
 INFO  [bind10.log] LOG_LOCK_TEST_MESSAGE this is a test message.
 LOGGER_LOCK_TEST: UNLOCK
-LOGGER_LOCK_TEST: UNLOCK
 .
 rm -f $destfile
 B10_LOGGER_SEVERITY=INFO B10_LOGGER_DESTINATION=stdout ./logger_lock_test > $destfile

diff --git a/src/lib/util/interprocess_sync.h b/src/lib/util/interprocess_sync.h
index b04c37fb13563ac5aadc6000333dcfaffe47c57c..bcf46c06550af016178882bb04c2bdc1c7805aed 100644 (file)
--- a/src/lib/util/interprocess_sync.h
+++ b/src/lib/util/interprocess_sync.h
@@ -38,6 +38,10 @@ class InterprocessSyncLocker; // forward declaration
 /// 4. Client performs task that needs mutual exclusion.
 /// 5. Client frees lock with unlock(), or simply returns from the basic
 /// block which forms the scope for the InterprocessSyncLocker.
+///
+/// NOTE: All implementations of InterprocessSync should keep the
+/// is_locked_ member variable updated whenever their
+/// lock()/tryLock()/unlock() implementations are called.
 class InterprocessSync {
   // InterprocessSyncLocker is the only code outside this class that
   // should be allowed to call the lock(), tryLock() and unlock()
@@ -100,7 +104,8 @@ public:
 
     /// \brief Destructor
     ~InterprocessSyncLocker() {
-        unlock();
+        if (isLocked())
+            unlock();
     }
 
     /// \brief Acquire the lock (blocks if something else has acquired a
@@ -113,11 +118,20 @@ public:
 
     /// \brief Try to acquire a lock (doesn't block)
     ///
-    /// \return Returns true if the lock was acquired, false otherwise.
+    /// \return Returns true if a new lock could be acquired, false
+    ///         otherwise.
     bool tryLock() {
         return (sync_.tryLock());
     }
 
+    /// \brief Check if the lock is taken
+    ///
+    /// \return Returns true if a lock is currently acquired, false
+    ///         otherwise.
+    bool isLocked() {
+        return (sync_.is_locked_);
+    }
+
     /// \brief Release the lock
     ///
     /// \return Returns true if the lock was released, false otherwise.

diff --git a/src/lib/util/interprocess_sync_null.cc b/src/lib/util/interprocess_sync_null.cc
index e2cfc9b95af4dff636aaf4a12d95114f1e270150..5355d5727e9ae25a021c41f9e078abc700372f9c 100644 (file)
--- a/src/lib/util/interprocess_sync_null.cc
+++ b/src/lib/util/interprocess_sync_null.cc
@@ -22,16 +22,19 @@ InterprocessSyncNull::~InterprocessSyncNull() {
 
 bool
 InterprocessSyncNull::lock() {
+    is_locked_ = true;
     return (true);
 }
 
 bool
 InterprocessSyncNull::tryLock() {
+    is_locked_ = true;
     return (true);
 }
 
 bool
 InterprocessSyncNull::unlock() {
+    is_locked_ = false;
     return (true);
 }
 

diff --git a/src/lib/util/tests/interprocess_sync_file_unittest.cc b/src/lib/util/tests/interprocess_sync_file_unittest.cc
index 46f96cc6aae9bc304e6e8c297938054d7f0ed95a..a0b41092030fe0dcc29b209721de98522f0d6512 100644 (file)
--- a/src/lib/util/tests/interprocess_sync_file_unittest.cc
+++ b/src/lib/util/tests/interprocess_sync_file_unittest.cc
@@ -55,7 +55,9 @@ TEST(InterprocessSyncFileTest, TestLock) {
   InterprocessSyncFile sync("test");
   InterprocessSyncLocker locker(sync);
 
+  EXPECT_FALSE(locker.isLocked());
   EXPECT_TRUE(locker.lock());
+  EXPECT_TRUE(locker.isLocked());
 
   int fds[2];
 
@@ -77,7 +79,10 @@ TEST(InterprocessSyncFileTest, TestLock) {
       InterprocessSyncLocker locker2(sync2);
 
       if (!locker2.tryLock()) {
+          EXPECT_FALSE(locker2.isLocked());
           locked = 1;
+      } else {
+          EXPECT_TRUE(locker2.isLocked());
       }
 
       write(fds[1], &locked, sizeof(locked));
@@ -95,6 +100,7 @@ TEST(InterprocessSyncFileTest, TestLock) {
   }
 
   EXPECT_TRUE(locker.unlock());
+  EXPECT_FALSE(locker.isLocked());
 
   EXPECT_EQ (0, unlink(TEST_DATA_TOPBUILDDIR "/test_lockfile"));
 }

diff --git a/src/lib/util/tests/interprocess_sync_null_unittest.cc b/src/lib/util/tests/interprocess_sync_null_unittest.cc
index 81cff76aaf848822e0990deb23a79d1ea49d0bc5..70e2b07f0e36b77a5f57a6a9db5740d79f0d57bd 100644 (file)
--- a/src/lib/util/tests/interprocess_sync_null_unittest.cc
+++ b/src/lib/util/tests/interprocess_sync_null_unittest.cc
@@ -24,28 +24,43 @@ TEST(InterprocessSyncNullTest, TestNull) {
   InterprocessSyncNull sync("test1");
   InterprocessSyncLocker locker(sync);
 
-  // All of these must always return true
-
+  // Check if the is_locked_ flag is set correctly during lock().
+  EXPECT_FALSE(locker.isLocked());
   EXPECT_TRUE(locker.lock());
+  EXPECT_TRUE(locker.isLocked());
+
+  // lock() must always return true (this is called 4 times, just an
+  // arbitrary number)
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.lock());
 
+  // Check if the is_locked_ flag is set correctly during unlock().
+  EXPECT_TRUE(locker.isLocked());
   EXPECT_TRUE(locker.unlock());
+  EXPECT_FALSE(locker.isLocked());
+
+  // unlock() must always return true (this is called 4 times, just an
+  // arbitrary number)
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.unlock());
 
+  // Check if the is_locked_ flag is set correctly during tryLock().
+  EXPECT_FALSE(locker.isLocked());
   EXPECT_TRUE(locker.tryLock());
+  EXPECT_TRUE(locker.isLocked());
+
+  // tryLock() must always return true (this is called 4 times, just an
+  // arbitrary number)
   EXPECT_TRUE(locker.tryLock());
   EXPECT_TRUE(locker.tryLock());
   EXPECT_TRUE(locker.tryLock());
   EXPECT_TRUE(locker.tryLock());
 
-  // Random order
-
+  // Random order (should all return true)
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.tryLock());