Bug 5169: StoreMap.cc:517 "!s.reading()" assertion (#918)

unlockSharedAndSwitchToExclusive() was not properly failing when future
readers violated preconditions for obtaining an exclusive lock:

- `if (!readers)` means no old readers
+ `if (!readLevel)` means no old readers and nobody is becoming a reader

That missing "becoming a reader" condition covers a lockShared() caller
that had passed their writeLevel test before we incremented writeLevel
to lock other lockShared() callers out. That caller increments `readers`
while unlockSharedAndSwitchToExclusive() makes `writing` true.

Introduced in commit 1af789e due to poor code duplication. This change
also removes that code duplication by adding finalizeExclusive().

diff --git a/src/ipc/ReadWriteLock.cc b/src/ipc/ReadWriteLock.cc
index aa5e60ce5775178424483135ff96e92f39fae14d..041d4f5deaf90aecd8239fa15a65e212ae3b5a8b 100644 (file)
--- a/src/ipc/ReadWriteLock.cc
+++ b/src/ipc/ReadWriteLock.cc
@@ -20,6 +20,23 @@ void Ipc::AssertFlagIsSet(std::atomic_flag &flag)
     assert(flag.test_and_set(std::memory_order_relaxed));
 }
 
+/// common lockExclusive() and unlockSharedAndSwitchToExclusive() logic:
+/// either finish exclusive locking or bail properly
+/// \pre The caller must (be the first to) increment writeLevel.
+/// \returns whether we got the exclusive lock
+bool
+Ipc::ReadWriteLock::finalizeExclusive()
+{
+    assert(writeLevel); // "new" readers are locked out by the caller
+    assert(!appending); // nobody can be appending without an exclusive lock
+    if (!readLevel) { // no old readers and nobody is becoming a reader
+        writing = true;
+        return true;
+    }
+    --writeLevel;
+    return false;
+}
+
 bool
 Ipc::ReadWriteLock::lockShared()
 {
@@ -35,12 +52,9 @@ Ipc::ReadWriteLock::lockShared()
 bool
 Ipc::ReadWriteLock::lockExclusive()
 {
-    if (!writeLevel++) { // we are the first writer + lock "new" readers out
-        if (!readLevel) { // no old readers and nobody is becoming one
-            writing = true;
-            return true;
-        }
-    }
+    if (!writeLevel++) // we are the first writer + lock "new" readers out
+        return finalizeExclusive(); // decrements writeLevel on failures
+
     --writeLevel;
     return false;
 }
@@ -96,17 +110,12 @@ Ipc::ReadWriteLock::unlockSharedAndSwitchToExclusive()
 {
     assert(readers > 0);
     if (!writeLevel++) { // we are the first writer + lock "new" readers out
-        assert(!appending);
-        unlockShared();
-        if (!readers) {
-            writing = true;
-            return true;
-        }
-        // somebody is still reading: fall through
-    } else {
-        // somebody is still writing: just stop reading
         unlockShared();
+        return finalizeExclusive(); // decrements writeLevel on failures
     }
+
+    // somebody is still writing, so we just stop reading
+    unlockShared();
     --writeLevel;
     return false;
 }

diff --git a/src/ipc/ReadWriteLock.h b/src/ipc/ReadWriteLock.h
index 7ad62a99bae1d21c93d69eb1b9210a3b95121166..b79eb682c36ff28d95311a40c560a90564d24c51 100644 (file)
--- a/src/ipc/ReadWriteLock.h
+++ b/src/ipc/ReadWriteLock.h
@@ -52,6 +52,8 @@ public:
     std::atomic_flag updating; ///< a reader is updating metadata/headers
 
 private:
+    bool finalizeExclusive();
+
     mutable std::atomic<uint32_t> readLevel; ///< number of users reading (or trying to)
     std::atomic<uint32_t> writeLevel; ///< number of users writing (or trying to write)
 };