From ccd4e41a99639913fdd3cc44d1bd0e4d4a81ab1a Mon Sep 17 00:00:00 2001
From: Oliver Hamlet <git@ortham.net>
Date: Fri, 28 Feb 2025 18:59:42 +0000
Subject: [PATCH] Improve game cache locking

The mutex in the game cache wasn't very effective, instead move the
mutex out to the only place that concurrently writes to the cache.

This does mean that there isn't protection against calling
GameInterface::LoadPlugins() concurrently from multiple threads, but
no effort has been made to make libloot's public API thread-safe anyway.
---
 src/api/game/game.cpp       |  3 +++
 src/api/game/game_cache.cpp | 53 -------------------------------------
 src/api/game/game_cache.h   | 11 --------
 3 files changed, 3 insertions(+), 64 deletions(-)

diff --git a/src/api/game/game.cpp b/src/api/game/game.cpp
index c9aca43b..725582a2 100644
--- a/src/api/game/game.cpp
+++ b/src/api/game/game.cpp
@@ -263,6 +263,7 @@ void Game::LoadPlugins(const std::vector<std::filesystem::path>& pluginPaths,
     logger->trace("Starting plugin loading.");
   }
 
+  std::mutex mutex;
   std::for_each(
       std::execution::par_unseq,
       pluginPaths.begin(),
@@ -272,6 +273,8 @@ void Game::LoadPlugins(const std::vector<std::filesystem::path>& pluginPaths,
           const auto resolvedPluginPath =
               ResolvePluginPath(GetType(), DataPath(), pluginPath);
 
+          std::lock_guard<std::mutex> lock(mutex);
+
           cache_.AddPlugin(
               Plugin(GetType(), cache_, resolvedPluginPath, loadHeadersOnly));
         } catch (const std::exception& e) {
diff --git a/src/api/game/game_cache.cpp b/src/api/game/game_cache.cpp
index b2b7868f..2fef1751 100644
--- a/src/api/game/game_cache.cpp
+++ b/src/api/game/game_cache.cpp
@@ -24,53 +24,10 @@
 
 #include "api/game/game_cache.h"
 
-#include <thread>
-
 #include "api/helpers/text.h"
 
-using std::lock_guard;
-using std::mutex;
-
 namespace loot {
-GameCache::GameCache(const GameCache& cache) {
-  lock_guard<mutex> lock(cache.mutex_);
-
-  plugins_ = cache.plugins_;
-  archivePaths_ = cache.archivePaths_;
-}
-
-GameCache::GameCache(GameCache&& cache) {
-  lock_guard<mutex> lock(cache.mutex_);
-
-  plugins_ = std::move(cache.plugins_);
-  archivePaths_ = std::move(cache.archivePaths_);
-}
-
-GameCache& GameCache::operator=(const GameCache& cache) {
-  if (&cache != this) {
-    std::scoped_lock lock(mutex_, cache.mutex_);
-
-    plugins_ = cache.plugins_;
-    archivePaths_ = cache.archivePaths_;
-  }
-
-  return *this;
-}
-
-GameCache& GameCache::operator=(GameCache&& cache) {
-  if (&cache != this) {
-    std::scoped_lock lock(mutex_, cache.mutex_);
-
-    plugins_ = std::move(cache.plugins_);
-    archivePaths_ = std::move(cache.archivePaths_);
-  }
-
-  return *this;
-}
-
 std::vector<const Plugin*> GameCache::GetPlugins() const {
-  lock_guard<mutex> lock(mutex_);
-
   std::vector<const Plugin*> output(plugins_.size());
   std::transform(
       begin(plugins_), end(plugins_), begin(output), [](const auto& pair) {
@@ -80,8 +37,6 @@ std::vector<const Plugin*> GameCache::GetPlugins() const {
 }
 
 const Plugin* GameCache::GetPlugin(const std::string& pluginName) const {
-  lock_guard<mutex> lock(mutex_);
-
   const auto it = plugins_.find(NormalizeFilename(pluginName));
   if (it != end(plugins_))
     return it->second.get();
@@ -90,8 +45,6 @@ const Plugin* GameCache::GetPlugin(const std::string& pluginName) const {
 }
 
 void GameCache::AddPlugin(Plugin&& plugin) {
-  lock_guard<mutex> lock(mutex_);
-
   auto normalizedName = NormalizeFilename(plugin.GetName());
   auto pluginPointer = std::make_shared<Plugin>(std::move(plugin));
 
@@ -104,20 +57,14 @@ void GameCache::AddPlugin(Plugin&& plugin) {
 }
 
 std::set<std::filesystem::path> GameCache::GetArchivePaths() const {
-  lock_guard<mutex> lock(mutex_);
-
   return archivePaths_;
 }
 
 void GameCache::CacheArchivePaths(std::set<std::filesystem::path>&& paths) {
-  lock_guard<mutex> lock(mutex_);
-
   archivePaths_ = std::move(paths);
 }
 
 void GameCache::ClearCachedPlugins() {
-  lock_guard<mutex> guard(mutex_);
-
   plugins_.clear();
 }
 }
diff --git a/src/api/game/game_cache.h b/src/api/game/game_cache.h
index 33f30683..343d7c0f 100644
--- a/src/api/game/game_cache.h
+++ b/src/api/game/game_cache.h
@@ -25,7 +25,6 @@
 #ifndef LOOT_API_GAME_GAME_CACHE
 #define LOOT_API_GAME_GAME_CACHE
 
-#include <mutex>
 #include <string>
 #include <unordered_map>
 
@@ -34,14 +33,6 @@
 namespace loot {
 class GameCache {
 public:
-  GameCache() = default;
-  GameCache(const GameCache& cache);
-  GameCache(GameCache&& cache);
-  ~GameCache() = default;
-
-  GameCache& operator=(const GameCache& cache);
-  GameCache& operator=(GameCache&& cache);
-
   std::vector<const Plugin*> GetPlugins() const;
   const Plugin* GetPlugin(const std::string& pluginName) const;
   void AddPlugin(Plugin&& plugin);
@@ -54,8 +45,6 @@ public:
 private:
   std::unordered_map<std::string, std::shared_ptr<const Plugin>> plugins_;
   std::set<std::filesystem::path> archivePaths_;
-
-  mutable std::mutex mutex_;
 };
 }