Fix locks for global resources by implementing a mutex not tied to a thread

* Can not use a normal mutex because we don't want to tie the resources to
  a specific thread (and instead e.g. to a build action which might not be
  executed by a single thread)
* A semaphore would do that but libstdc++ only supports it as of GCC 11 and
  besides it wouldn't distinguish between shared and exclusive locking

librepomgr/CMakeLists.txt

@@ -9,7 +9,7 @@ set(HEADER_FILES
     logcontext.h
     logging.h
     multisession.h
-    namedlockable.h
+    globallock.h
     authentication.h
     webapi/server.h
     webapi/session.h
@@ -32,7 +32,7 @@ set(SRC_FILES
     json.cpp
     errorhandling.cpp
     serversetup.cpp
-    namedlockable.cpp
+    globallock.cpp
     authentication.cpp
     webapi/server.cpp
     webapi/session.cpp
@@ -58,7 +58,7 @@ set(SRC_FILES
     buildactions/preparebuild.cpp
     buildactions/conductbuild.cpp)
 set(TEST_HEADER_FILES tests/parser_helper.h)
-set(TEST_SRC_FILES tests/cppunit.cpp tests/buildactions.cpp tests/webapi.cpp tests/parser_helper.cpp)
+set(TEST_SRC_FILES tests/cppunit.cpp tests/buildactions.cpp tests/utils.cpp tests/webapi.cpp tests/parser_helper.cpp)
 
 # meta data
 set(META_PROJECT_NAME librepomgr)

librepomgr/buildactions/buildaction.h

@@ -7,8 +7,8 @@
 
 #include "../webapi/routes.h"
 
+#include "../globallock.h"
 #include "../logcontext.h"
-#include "../namedlockable.h"
 
 #include "../../libpkg/data/config.h"
 #include "../../libpkg/data/lockable.h"
@@ -47,7 +47,7 @@ class Session;
 
 struct InternalBuildAction;
 
-using AssociatedLocks = std::vector<std::variant<SharedNamedLock, UniqueNamedLock>>;
+using AssociatedLocks = std::vector<std::variant<SharedLoggingLock, UniqueLoggingLock>>;
 
 struct LIBREPOMGR_EXPORT PackageBuildData : public ReflectiveRapidJSON::JsonSerializable<PackageBuildData>,
                                             public ReflectiveRapidJSON::BinarySerializable<PackageBuildData> {

librepomgr/globallock.cpp

@@ -0,0 +1,9 @@
+#include "./globallock.h"
+#include "./logging.h"
+
+namespace LibRepoMgr {
+
+template struct LoggingLock<std::shared_lock<GlobalSharedMutex>>;
+template struct LoggingLock<std::unique_lock<GlobalSharedMutex>>;
+
+} // namespace LibRepoMgr

librepomgr/globallock.h

@@ -0,0 +1,178 @@
+#ifndef LIBREPOMGR_GLOBAL_LOCK_H
+#define LIBREPOMGR_GLOBAL_LOCK_H
+
+#include <cstdint>
+#include <mutex>
+#include <shared_mutex>
+
+#include "./global.h"
+
+namespace LibRepoMgr {
+
+struct LogContext;
+
+/// \brief A shared mutex where ownership is not tied to a thread (similar to a binary semaphore in that regard).
+struct GlobalSharedMutex {
+    void lock();
+    bool try_lock();
+    void unlock();
+
+    void lock_shared();
+    bool try_lock_shared();
+    void unlock_shared();
+
+private:
+    std::mutex m_mutex;
+    std::condition_variable m_cv;
+    std::uint32_t m_sharedOwners = 0;
+    bool m_exclusivelyOwned = false;
+};
+
+inline void GlobalSharedMutex::lock()
+{
+    auto lock = std::unique_lock<std::mutex>(m_mutex);
+    if (m_sharedOwners || m_exclusivelyOwned) {
+        m_cv.wait(lock);
+    }
+    m_exclusivelyOwned = true;
+}
+
+inline bool GlobalSharedMutex::try_lock()
+{
+    auto lock = std::unique_lock<std::mutex>(m_mutex);
+    if (m_sharedOwners || m_exclusivelyOwned) {
+        return false;
+    } else {
+        return m_exclusivelyOwned = true;
+    }
+}
+
+inline void GlobalSharedMutex::unlock()
+{
+    m_exclusivelyOwned = false;
+    m_cv.notify_one();
+}
+
+inline void GlobalSharedMutex::lock_shared()
+{
+    auto lock = std::unique_lock<std::mutex>(m_mutex);
+    if (m_exclusivelyOwned) {
+        m_cv.wait(lock);
+    }
+    ++m_sharedOwners;
+}
+
+inline bool GlobalSharedMutex::try_lock_shared()
+{
+    auto lock = std::unique_lock<std::mutex>(m_mutex);
+    if (m_exclusivelyOwned) {
+        return false;
+    } else {
+        return ++m_sharedOwners;
+    }
+}
+
+inline void GlobalSharedMutex::unlock_shared()
+{
+    auto lock = std::unique_lock<std::mutex>(m_mutex);
+    if (!--m_sharedOwners) {
+        lock.unlock();
+        m_cv.notify_one();
+    }
+}
+
+/// \brief A wrapper around a standard lock which logs acquisition/release.
+template <typename UnderlyingLockType> struct LoggingLock {
+    template <typename... Args> LoggingLock(LogContext &log, std::string &&name, Args &&...args);
+    LoggingLock(LoggingLock &&) = default;
+    ~LoggingLock();
+
+    const std::string &name() const
+    {
+        return m_name;
+    };
+    UnderlyingLockType &lock()
+    {
+        return m_lock;
+    };
+
+private:
+    LogContext &m_log;
+    std::string m_name;
+    UnderlyingLockType m_lock;
+};
+
+constexpr std::string_view lockName(std::shared_lock<GlobalSharedMutex> &)
+{
+    return "shared";
+}
+constexpr std::string_view lockName(std::unique_lock<GlobalSharedMutex> &)
+{
+    return "exclusive";
+}
+
+template <typename UnderlyingLockType>
+template <typename... Args>
+inline LoggingLock<UnderlyingLockType>::LoggingLock(LogContext &log, std::string &&name, Args &&...args)
+    : m_log(log)
+    , m_name(std::move(name))
+{
+    m_log("Acquiring ", lockName(m_lock), " lock \"", m_name, "\"\n");
+    m_lock = UnderlyingLockType(std::forward<Args>(args)...);
+}
+
+template <typename UnderlyingLockType> inline LoggingLock<UnderlyingLockType>::~LoggingLock()
+{
+    if (m_lock) {
+        m_lock.unlock();
+        m_log("Released ", lockName(m_lock), " lock \"", m_name, "\"\n");
+    }
+}
+
+using SharedLoggingLock = LoggingLock<std::shared_lock<GlobalSharedMutex>>;
+using UniqueLoggingLock = LoggingLock<std::unique_lock<GlobalSharedMutex>>;
+
+extern template struct LIBREPOMGR_EXPORT LoggingLock<std::shared_lock<GlobalSharedMutex>>;
+extern template struct LIBREPOMGR_EXPORT LoggingLock<std::unique_lock<GlobalSharedMutex>>;
+
+/// \brief Same as LibPkg::Lockable but using GlobalSharedMutex.
+struct GlobalLockable {
+    [[nodiscard]] SharedLoggingLock lockToRead(LogContext &log, std::string &&name) const;
+    [[nodiscard]] UniqueLoggingLock lockToWrite(LogContext &log, std::string &&name);
+    [[nodiscard]] SharedLoggingLock tryLockToRead(LogContext &log, std::string &&name) const;
+    [[nodiscard]] UniqueLoggingLock tryLockToWrite(LogContext &log, std::string &&name);
+    [[nodiscard]] UniqueLoggingLock lockToWrite(LogContext &log, std::string &&name, SharedLoggingLock &readLock);
+
+private:
+    mutable GlobalSharedMutex m_mutex;
+};
+
+inline SharedLoggingLock GlobalLockable::lockToRead(LogContext &log, std::string &&name) const
+{
+    return SharedLoggingLock(log, std::move(name), m_mutex);
+}
+
+inline UniqueLoggingLock GlobalLockable::lockToWrite(LogContext &log, std::string &&name)
+{
+    return UniqueLoggingLock(log, std::move(name), m_mutex);
+}
+
+inline SharedLoggingLock GlobalLockable::tryLockToRead(LogContext &log, std::string &&name) const
+{
+    return SharedLoggingLock(log, std::move(name), m_mutex, std::try_to_lock);
+}
+
+inline UniqueLoggingLock GlobalLockable::tryLockToWrite(LogContext &log, std::string &&name)
+{
+    return UniqueLoggingLock(log, std::move(name), m_mutex, std::try_to_lock);
+}
+
+inline UniqueLoggingLock GlobalLockable::lockToWrite(LogContext &log, std::string &&name, SharedLoggingLock &readLock)
+{
+    readLock.lock().unlock();
+    return UniqueLoggingLock(log, std::move(name), m_mutex);
+}
+
+} // namespace LibRepoMgr
+
+#endif // LIBREPOMGR_GLOBAL_LOCK_H

librepomgr/namedlockable.cpp

@@ -1,9 +0,0 @@
-#include "./namedlockable.h"
-#include "./logging.h"
-
-namespace LibRepoMgr {
-
-template struct NamedLock<std::shared_lock<std::shared_mutex>>;
-template struct NamedLock<std::unique_lock<std::shared_mutex>>;
-
-} // namespace LibRepoMgr

librepomgr/namedlockable.h

@@ -1,105 +0,0 @@
-#ifndef LIBREPOMGR_NAMED_LOCKABLE_H
-#define LIBREPOMGR_NAMED_LOCKABLE_H
-
-#include <mutex>
-#include <shared_mutex>
-
-#include "./global.h"
-
-namespace LibRepoMgr {
-
-struct LogContext;
-
-template <typename UnderlyingLockType> struct NamedLock {
-    template <typename... Args> NamedLock(LogContext &log, std::string &&name, Args &&...args);
-    NamedLock(NamedLock &&) = default;
-    ~NamedLock();
-
-    const std::string &name() const
-    {
-        return m_name;
-    };
-    UnderlyingLockType &lock()
-    {
-        return m_lock;
-    };
-
-private:
-    LogContext &m_log;
-    std::string m_name;
-    UnderlyingLockType m_lock;
-};
-
-constexpr std::string_view lockName(std::shared_lock<std::shared_mutex> &)
-{
-    return "shared";
-}
-constexpr std::string_view lockName(std::unique_lock<std::shared_mutex> &)
-{
-    return "exclusive";
-}
-
-template <typename UnderlyingLockType>
-template <typename... Args>
-inline NamedLock<UnderlyingLockType>::NamedLock(LogContext &log, std::string &&name, Args &&...args)
-    : m_log(log)
-    , m_name(std::move(name))
-{
-    m_log("Acquiring ", lockName(m_lock), " lock \"", m_name, "\"\n");
-    m_lock = UnderlyingLockType(std::forward<Args>(args)...);
-}
-
-template <typename UnderlyingLockType> inline NamedLock<UnderlyingLockType>::~NamedLock()
-{
-    if (m_lock) {
-        m_lock.unlock();
-        m_log("Released ", lockName(m_lock), " lock \"", m_name, "\"\n");
-    }
-}
-
-using SharedNamedLock = NamedLock<std::shared_lock<std::shared_mutex>>;
-using UniqueNamedLock = NamedLock<std::unique_lock<std::shared_mutex>>;
-
-extern template struct LIBREPOMGR_EXPORT NamedLock<std::shared_lock<std::shared_mutex>>;
-extern template struct LIBREPOMGR_EXPORT NamedLock<std::unique_lock<std::shared_mutex>>;
-
-struct NamedLockable {
-    [[nodiscard]] SharedNamedLock lockToRead(LogContext &log, std::string &&name) const;
-    [[nodiscard]] UniqueNamedLock lockToWrite(LogContext &log, std::string &&name);
-    [[nodiscard]] SharedNamedLock tryLockToRead(LogContext &log, std::string &&name) const;
-    [[nodiscard]] UniqueNamedLock tryLockToWrite(LogContext &log, std::string &&name);
-    [[nodiscard]] UniqueNamedLock lockToWrite(LogContext &log, std::string &&name, SharedNamedLock &readLock);
-
-private:
-    mutable std::shared_mutex m_mutex;
-};
-
-inline SharedNamedLock NamedLockable::lockToRead(LogContext &log, std::string &&name) const
-{
-    return SharedNamedLock(log, std::move(name), m_mutex);
-}
-
-inline UniqueNamedLock NamedLockable::lockToWrite(LogContext &log, std::string &&name)
-{
-    return UniqueNamedLock(log, std::move(name), m_mutex);
-}
-
-inline SharedNamedLock NamedLockable::tryLockToRead(LogContext &log, std::string &&name) const
-{
-    return SharedNamedLock(log, std::move(name), m_mutex, std::try_to_lock);
-}
-
-inline UniqueNamedLock NamedLockable::tryLockToWrite(LogContext &log, std::string &&name)
-{
-    return UniqueNamedLock(log, std::move(name), m_mutex, std::try_to_lock);
-}
-
-inline UniqueNamedLock NamedLockable::lockToWrite(LogContext &log, std::string &&name, SharedNamedLock &readLock)
-{
-    readLock.lock().unlock();
-    return UniqueNamedLock(log, std::move(name), m_mutex);
-}
-
-} // namespace LibRepoMgr
-
-#endif // LIBREPOMGR_NAMED_LOCKABLE_H

librepomgr/serversetup.h

@@ -4,7 +4,7 @@
 #include "./authentication.h"
 #include "./buildactions/buildaction.h"
 #include "./buildactions/buildactiontemplate.h"
-#include "./namedlockable.h"
+#include "./globallock.h"
 
 #include "../libpkg/data/config.h"
 #include "../libpkg/data/lockable.h"
@@ -122,15 +122,15 @@ struct LIBREPOMGR_EXPORT ServiceSetup : public LibPkg::Lockable {
     } auth;
 
     struct LIBREPOMGR_EXPORT Locks {
-        [[nodiscard]] SharedNamedLock acquireToRead(LogContext &log, std::string &&lockName);
-        [[nodiscard]] UniqueNamedLock acquireToWrite(LogContext &log, std::string &&lockName);
+        [[nodiscard]] SharedLoggingLock acquireToRead(LogContext &log, std::string &&lockName);
+        [[nodiscard]] UniqueLoggingLock acquireToWrite(LogContext &log, std::string &&lockName);
         void clear();
         static std::string forDatabase(std::string_view dbName, std::string_view dbArch);
         static std::string forDatabase(const LibPkg::Database &db);
 
     private:
         std::mutex m_mutex;
-        std::unordered_map<std::string, NamedLockable> m_locksByName;
+        std::unordered_map<std::string, GlobalLockable> m_locksByName;
     } locks;
 
     void loadConfigFiles(bool restoreStateAndDiscardDatabases);
@@ -149,13 +149,13 @@ inline std::shared_ptr<BuildAction> ServiceSetup::BuildSetup::getBuildAction(Bui
     return id < actions.size() ? actions[id] : nullptr;
 }
 
-inline SharedNamedLock ServiceSetup::Locks::acquireToRead(LogContext &log, std::string &&lockName)
+inline SharedLoggingLock ServiceSetup::Locks::acquireToRead(LogContext &log, std::string &&lockName)
 {
     const auto lock = std::lock_guard(m_mutex);
     return m_locksByName[lockName].lockToRead(log, std::move(lockName));
 }
 
-inline UniqueNamedLock ServiceSetup::Locks::acquireToWrite(LogContext &log, std::string &&lockName)
+inline UniqueLoggingLock ServiceSetup::Locks::acquireToWrite(LogContext &log, std::string &&lockName)
 {
     const auto lock = std::lock_guard(m_mutex);
     return m_locksByName[lockName].lockToWrite(log, std::move(lockName));

librepomgr/tests/utils.cpp

@@ -0,0 +1,71 @@
+#include "../globallock.h"
+#include "../logging.h"
+
+#include <c++utilities/conversion/stringbuilder.h>
+#include <c++utilities/io/misc.h>
+#include <c++utilities/tests/testutils.h>
+
+#include <cppunit/TestFixture.h>
+#include <cppunit/extensions/HelperMacros.h>
+
+#include <thread>
+
+using namespace std;
+using namespace CPPUNIT_NS;
+using namespace CppUtilities;
+using namespace CppUtilities::Literals;
+
+using namespace LibRepoMgr;
+
+class UtilsTests : public TestFixture {
+    CPPUNIT_TEST_SUITE(UtilsTests);
+    CPPUNIT_TEST(testGlobalLock);
+    CPPUNIT_TEST_SUITE_END();
+
+    void testGlobalLock();
+
+public:
+    UtilsTests();
+    void setUp() override;
+    void tearDown() override;
+
+private:
+};
+
+CPPUNIT_TEST_SUITE_REGISTRATION(UtilsTests);
+
+UtilsTests::UtilsTests()
+{
+}
+
+void UtilsTests::setUp()
+{
+}
+
+void UtilsTests::tearDown()
+{
+}
+
+void UtilsTests::testGlobalLock()
+{
+    auto mutex = GlobalSharedMutex();
+    auto sharedLock1 = std::shared_lock(mutex);
+    auto sharedLock2 = std::shared_lock(mutex); // locking twice is not a problem, also not from the same thread
+    auto thread1 = std::thread([&sharedLock1] {
+        sharedLock1.unlock(); // unlocking from another thread is ok
+    });
+    auto thread2 = std::thread([&mutex] { mutex.lock(); });
+    sharedLock2.unlock();
+    thread1.join();
+    thread2.join(); // thread2 should be able to acquire the mutex exclusively (and then terminate)
+    CPPUNIT_ASSERT_MESSAGE("try_lock_shared() returns false if mutex exclusively locked", !mutex.try_lock_shared());
+    auto thread3 = std::thread([&mutex] { mutex.lock_shared(); });
+    mutex.unlock();
+    thread3.join(); // thread3 should be able to acquire the mutex (and then terminate)
+    CPPUNIT_ASSERT_MESSAGE("try_lock_shared() possible if mutex only shared locked", mutex.try_lock_shared());
+    mutex.unlock_shared();
+    CPPUNIT_ASSERT_MESSAGE("try_lock() returns false if mutex has still shared locked", !mutex.try_lock());
+    mutex.unlock_shared();
+    CPPUNIT_ASSERT_MESSAGE("try_lock() possible if mutex not locked", mutex.try_lock());
+    mutex.unlock();
+}