Make AccessGuard use a shared_mutex instead (#2702)

This allows `accessConst` to use a shared lock instead of a unique lock

src/common/ChannelChatters.cpp

@@ -11,7 +11,7 @@ ChannelChatters::ChannelChatters(Channel &channel)
 {
 }
 
-AccessGuard<const UsernameSet> ChannelChatters::accessChatters() const
+SharedAccessGuard<const UsernameSet> ChannelChatters::accessChatters() const
 {
     return this->chatters_.accessConst();
 }

src/common/ChannelChatters.hpp

@@ -15,7 +15,7 @@ public:
     ChannelChatters(Channel &channel);
     virtual ~ChannelChatters() = default;  // add vtable
 
-    AccessGuard<const UsernameSet> accessChatters() const;
+    SharedAccessGuard<const UsernameSet> accessChatters() const;
 
     void addRecentChatter(const QString &user);
     void addJoinedUser(const QString &user);

src/common/UniqueAccess.hpp

@@ -1,39 +1,33 @@
 #pragma once
 
 #include <mutex>
+#include <shared_mutex>
 #include <type_traits>
 
 namespace chatterino {
 
-template <typename T>
+template <typename T, typename LockType = std::unique_lock<std::shared_mutex>>
 class AccessGuard
 {
 public:
-    AccessGuard(T &element, std::mutex &mutex)
+    AccessGuard(T &element, std::shared_mutex &mutex)
         : element_(&element)
-        , mutex_(&mutex)
+        , lock_(mutex)
     {
-        this->mutex_->lock();
     }
 
-    AccessGuard(AccessGuard<T> &&other)
+    AccessGuard(AccessGuard<T, LockType> &&other)
         : element_(other.element_)
-        , mutex_(other.mutex_)
+        , lock_(std::move(other.lock_))
     {
-        other.isValid_ = false;
     }
 
-    AccessGuard<T> &operator=(AccessGuard<T> &&other)
+    AccessGuard<T, LockType> &operator=(AccessGuard<T, LockType> &&other)
     {
-        other.isValid_ = false;
         this->element_ = other.element_;
-        this->mutex_ = other.element_;
+        this->lock_ = std::move(other.lock_);
-    }
 
-    ~AccessGuard()
+        return *this;
-    {
-        if (this->isValid_)
-            this->mutex_->unlock();
     }
 
     T *operator->() const
@@ -48,10 +42,13 @@ public:
 
 private:
     T *element_{};
-    std::mutex *mutex_{};
+    LockType lock_;
-    bool isValid_{true};
 };
 
+template <typename T>
+using SharedAccessGuard =
+    AccessGuard<const T, std::shared_lock<std::shared_mutex>>;
+
 template <typename T>
 class UniqueAccess
 {
@@ -90,14 +87,14 @@ public:
 
     template <typename X = T,
               typename = std::enable_if_t<!std::is_const<X>::value>>
-    AccessGuard<const X> accessConst() const
+    SharedAccessGuard<const X> accessConst() const
     {
-        return AccessGuard<const T>(this->element_, this->mutex_);
+        return SharedAccessGuard<const T>(this->element_, this->mutex_);
     }
 
 private:
     mutable T element_;
-    mutable std::mutex mutex_;
+    mutable std::shared_mutex mutex_;
 };
 
 }  // namespace chatterino

src/providers/twitch/TwitchAccount.cpp

@@ -176,12 +176,14 @@ void TwitchAccount::checkFollow(const QString targetUserID,
                               [] {});
 }
 
-AccessGuard<const std::set<TwitchUser>> TwitchAccount::accessBlocks() const
+SharedAccessGuard<const std::set<TwitchUser>> TwitchAccount::accessBlocks()
+    const
 {
     return this->ignores_.accessConst();
 }
 
-AccessGuard<const std::set<QString>> TwitchAccount::accessBlockedUserIds() const
+SharedAccessGuard<const std::set<QString>> TwitchAccount::accessBlockedUserIds()
+    const
 {
     return this->ignoresUserIds_.accessConst();
 }
@@ -345,7 +347,7 @@ void TwitchAccount::loadUserstateEmotes(QStringList emoteSetKeys)
         });
 }
 
-AccessGuard<const TwitchAccount::TwitchAccountEmoteData>
+SharedAccessGuard<const TwitchAccount::TwitchAccountEmoteData>
     TwitchAccount::accessEmotes() const
 {
     return this->emotes_.accessConst();

src/providers/twitch/TwitchAccount.hpp

@@ -106,12 +106,12 @@ public:
     void checkFollow(const QString targetUserID,
                      std::function<void(FollowResult)> onFinished);
 
-    AccessGuard<const std::set<QString>> accessBlockedUserIds() const;
+    SharedAccessGuard<const std::set<QString>> accessBlockedUserIds() const;
-    AccessGuard<const std::set<TwitchUser>> accessBlocks() const;
+    SharedAccessGuard<const std::set<TwitchUser>> accessBlocks() const;
 
     void loadEmotes();
     void loadUserstateEmotes(QStringList emoteSetKeys);
-    AccessGuard<const TwitchAccountEmoteData> accessEmotes() const;
+    SharedAccessGuard<const TwitchAccountEmoteData> accessEmotes() const;
 
     // Automod actions
     void autoModAllow(const QString msgID);

src/providers/twitch/TwitchChannel.cpp

@@ -454,8 +454,8 @@ void TwitchChannel::setRoomId(const QString &id)
     }
 }
 
-AccessGuard<const TwitchChannel::RoomModes> TwitchChannel::accessRoomModes()
+SharedAccessGuard<const TwitchChannel::RoomModes>
-    const
+    TwitchChannel::accessRoomModes() const
 {
     return this->roomModes_.accessConst();
 }
@@ -472,7 +472,7 @@ bool TwitchChannel::isLive() const
     return this->streamStatus_.access()->live;
 }
 
-AccessGuard<const TwitchChannel::StreamStatus>
+SharedAccessGuard<const TwitchChannel::StreamStatus>
     TwitchChannel::accessStreamStatus() const
 {
     return this->streamStatus_.accessConst();

src/providers/twitch/TwitchChannel.hpp

@@ -83,8 +83,8 @@ public:
     int chatterCount();
     virtual bool isLive() const override;
     QString roomId() const;
-    AccessGuard<const RoomModes> accessRoomModes() const;
+    SharedAccessGuard<const RoomModes> accessRoomModes() const;
-    AccessGuard<const StreamStatus> accessStreamStatus() const;
+    SharedAccessGuard<const StreamStatus> accessStreamStatus() const;
 
     // Emotes
     const TwitchBadges &globalTwitchBadges() const;

tests/CMakeLists.txt

@@ -36,6 +36,7 @@ set(chatterino_SOURCES
 
 set(test_SOURCES
     ${CMAKE_CURRENT_LIST_DIR}/src/main.cpp
+    ${CMAKE_CURRENT_LIST_DIR}/src/AccessGuard.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/NetworkCommon.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/NetworkRequest.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/UsernameSet.cpp

tests/src/AccessGuard.cpp

@@ -0,0 +1,76 @@
+#include "common/UniqueAccess.hpp"
+
+#include <gtest/gtest.h>
+#include <QDebug>
+#include <QString>
+
+#include <chrono>
+#include <random>
+#include <thread>
+
+using namespace chatterino;
+
+using namespace std::chrono_literals;
+
+TEST(AccessGuardLocker, NonConcurrentUsage)
+{
+    std::shared_mutex m;
+    int e = 0;
+
+    {
+        AccessGuard<int> guard(e, m);
+        *guard = 3;
+    }
+    EXPECT_EQ(e, 3);
+
+    {
+        AccessGuard<int> guard(e, m);
+        *guard = 4;
+    }
+    EXPECT_EQ(e, 4);
+
+    {
+        SharedAccessGuard<int> guard(e, m);
+        EXPECT_EQ(*guard, 4);
+    }
+    EXPECT_EQ(e, 4);
+}
+
+TEST(AccessGuardLocker, ConcurrentUsage)
+{
+    // This test doesn't actually prove anything on normal use, rather it needs to be run with AddressSanitizer/ThreadSanitizer and not error out for this to give any confidence
+    std::shared_mutex m;
+    int e = 0;
+
+    auto startTime = std::chrono::steady_clock::now();
+
+    auto w = [&e, &m] {
+        std::mt19937_64 eng{std::random_device{}()};
+        std::uniform_int_distribution<> dist{1, 4};
+        std::this_thread::sleep_for(std::chrono::milliseconds{dist(eng)});
+        if (rand() % 2 == 0)
+        {
+            AccessGuard<int> guard(e, m);
+            std::this_thread::sleep_for(std::chrono::milliseconds{dist(eng)});
+            *guard += 1;
+        }
+        else
+        {
+            SharedAccessGuard<int> guard(e, m);
+            std::this_thread::sleep_for(std::chrono::milliseconds{dist(eng)});
+            int hehe = *guard;
+        }
+    };
+
+    std::vector<std::thread> threads;
+
+    for (int i = 0; i < 500; ++i)
+    {
+        threads.emplace_back(w);
+    }
+
+    for (auto &t : threads)
+    {
+        t.join();
+    }
+}