From 57b4593b4029e9208bb0c5bcc3c0c9ec4d7236b2 Mon Sep 17 00:00:00 2001 From: Renato Machado Date: Thu, 2 Nov 2023 16:16:08 -0300 Subject: [PATCH] improve: use modern implementations to manage threads (#1756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This updates the thread management constructs within the codebase. It shifts from using std::lock_guard to std::scoped_lock for managing mutexes, providing a more modern and safer scope-based locking mechanism. Additionally, it replaces std::condition_variable with std::atomic::wait, leveraging the more efficient and scalable waiting and notification system provided by atomics. Benefits: • std::scoped_lock is more flexible than std::lock_guard, allowing for handling multiple mutexes without the risk of deadlock. • std::atomic::wait introduces potential performance gains by utilizing a more lightweight synchronization mechanism compared to traditional condition variables. Impact: • These improvements are expected to enhance the safety and efficiency of concurrent operations. They modernize the codebase, aligning it with current best practices for C++ concurrency. --- src/canary_server.cpp | 16 +++++++--------- src/canary_server.hpp | 15 +++++++-------- src/creatures/players/management/ban.cpp | 2 +- src/game/game.cpp | 2 +- src/kv/kv.cpp | 4 ++-- src/lib/di/shared.hpp | 8 ++++---- src/server/network/connection/connection.cpp | 14 +++++++------- 7 files changed, 29 insertions(+), 32 deletions(-) diff --git a/src/canary_server.cpp b/src/canary_server.cpp index 6d833e0ce0b..7facada14d5 100644 --- a/src/canary_server.cpp +++ b/src/canary_server.cpp @@ -40,8 +40,7 @@ CanaryServer::CanaryServer( ) : logger(logger), rsa(rsa), - serviceManager(serviceManager), - loaderUniqueLock(loaderLock) { + serviceManager(serviceManager) { logInfos(); toggleForceCloseButton(); g_game().setGameState(GAME_STATE_STARTUP); @@ -93,10 +92,9 @@ int CanaryServer::run() { g_webhook().sendMessage("Server is now online", "Server has successfully started.", WEBHOOK_COLOR_ONLINE); - loaderDone = true; - loaderSignal.notify_all(); + loaderStatus = LoaderStatus::LOADED; } catch (FailedToInitializeCanary &err) { - loadFailed = true; + loaderStatus = LoaderStatus::FAILED; logger.error(err.what()); logger.error("The program will close after pressing the enter key..."); @@ -104,16 +102,16 @@ int CanaryServer::run() { if (isatty(STDIN_FILENO)) { getchar(); } - - loaderSignal.notify_all(); } + + loaderStatus.notify_one(); }, "CanaryServer::run" ); - loaderSignal.wait(loaderUniqueLock, [this] { return loaderDone || loadFailed; }); + loaderStatus.wait(LoaderStatus::LOADING); - if (loadFailed || !serviceManager.is_running()) { + if (loaderStatus == LoaderStatus::FAILED || !serviceManager.is_running()) { logger.error("No services running. The server is NOT online!"); shutdown(); return EXIT_FAILURE; diff --git a/src/canary_server.hpp b/src/canary_server.hpp index 6d75f64dc16..d9374309c4a 100644 --- a/src/canary_server.hpp +++ b/src/canary_server.hpp @@ -40,18 +40,17 @@ class CanaryServer { int run(); private: + enum class LoaderStatus : uint8_t { + LOADING, + LOADED, + FAILED + }; + RSA &rsa; Logger &logger; ServiceManager &serviceManager; - std::mutex loaderLock; - std::condition_variable loaderSignal; - std::condition_variable mapSignal; - std::unique_lock loaderUniqueLock; - std::string threadFailMsg; - - bool loaderDone = false; - bool loadFailed = false; + std::atomic loaderStatus = LoaderStatus::LOADING; void logInfos(); static void toggleForceCloseButton(); diff --git a/src/creatures/players/management/ban.cpp b/src/creatures/players/management/ban.cpp index 0c08fdc6f1e..3315836ebea 100644 --- a/src/creatures/players/management/ban.cpp +++ b/src/creatures/players/management/ban.cpp @@ -15,7 +15,7 @@ #include "utils/tools.hpp" bool Ban::acceptConnection(uint32_t clientIP) { - std::lock_guard lockClass(lock); + std::scoped_lock lockClass(lock); uint64_t currentTime = OTSYS_TIME(); diff --git a/src/game/game.cpp b/src/game/game.cpp index 417c89a8497..a01ac60bff8 100644 --- a/src/game/game.cpp +++ b/src/game/game.cpp @@ -9864,7 +9864,7 @@ void Game::playerRewardChestCollect(uint32_t playerId, const Position &pos, uint reward->setParent(playerRewardChest); } - std::lock_guard lock(player->quickLootMutex); + std::scoped_lock lock(player->quickLootMutex); ReturnValue returnValue = collectRewardChestItems(player, maxMoveItems); if (returnValue != RETURNVALUE_NOERROR) { diff --git a/src/kv/kv.cpp b/src/kv/kv.cpp index e5dbc974b76..0e2f4b2f836 100644 --- a/src/kv/kv.cpp +++ b/src/kv/kv.cpp @@ -27,7 +27,7 @@ void KVStore::set(const std::string &key, const std::initializer_list KVStore::get(const std::string &key, bool forceLoad /*= false */) { logger.debug("KVStore::get({})", key); - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (forceLoad || !store_.contains(key)) { auto value = load(key); if (value) { diff --git a/src/lib/di/shared.hpp b/src/lib/di/shared.hpp index e206cbd4b4c..c1e2f0c6743 100644 --- a/src/lib/di/shared.hpp +++ b/src/lib/di/shared.hpp @@ -29,9 +29,9 @@ namespace extension { #if !defined(BOOST_DI_NOT_THREAD_SAFE) //<> explicit scope(scope &&other) noexcept : - scope(std::move(other), std::lock_guard(other.mutex_)) { } + scope(std::move(other), std::scoped_lock(other.mutex_)) { } //<> - scope(scope &&other, const std::lock_guard &) noexcept : + scope(scope &&other, const std::scoped_lock &) noexcept : object_(std::move(other.object_)) { } #endif @@ -49,7 +49,7 @@ namespace extension { wrappers::shared create(const TProvider &provider) & { if (!object_) { #if !defined(BOOST_DI_NOT_THREAD_SAFE) - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!object_) #endif object_ = std::shared_ptr { provider.get() }; @@ -65,7 +65,7 @@ namespace extension { auto &object = provider.cfg().template data(); if (!object) { #if !defined(BOOST_DI_NOT_THREAD_SAFE) - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!object) #endif object = std::shared_ptr { provider.get() }; diff --git a/src/server/network/connection/connection.cpp b/src/server/network/connection/connection.cpp index 8e637c053ee..d2937756cf0 100644 --- a/src/server/network/connection/connection.cpp +++ b/src/server/network/connection/connection.cpp @@ -53,7 +53,7 @@ void Connection::close(bool force) { // any thread ConnectionManager::getInstance().releaseConnection(shared_from_this()); - std::lock_guard lockClass(connectionLock); + std::scoped_lock lockClass(connectionLock); ip = 0; if (connectionState == CONNECTION_STATE_CLOSED) { return; @@ -114,7 +114,7 @@ void Connection::accept(bool toggleParseHeader /* = true */) { } void Connection::parseProxyIdentification(const std::error_code &error) { - std::lock_guard lockClass(connectionLock); + std::scoped_lock lockClass(connectionLock); readTimer.cancel(); if (error) { @@ -167,7 +167,7 @@ void Connection::parseProxyIdentification(const std::error_code &error) { } void Connection::parseHeader(const std::error_code &error) { - std::lock_guard lockClass(connectionLock); + std::scoped_lock lockClass(connectionLock); readTimer.cancel(); if (error) { @@ -209,7 +209,7 @@ void Connection::parseHeader(const std::error_code &error) { } void Connection::parsePacket(const std::error_code &error) { - std::lock_guard lockClass(connectionLock); + std::scoped_lock lockClass(connectionLock); readTimer.cancel(); if (error) { @@ -275,7 +275,7 @@ void Connection::parsePacket(const std::error_code &error) { } void Connection::resumeWork() { - std::lock_guard lockClass(connectionLock); + std::scoped_lock lockClass(connectionLock); try { // Wait to the next packet @@ -287,7 +287,7 @@ void Connection::resumeWork() { } void Connection::send(const OutputMessage_ptr &outputMessage) { - std::lock_guard lockClass(connectionLock); + std::scoped_lock lockClass(connectionLock); if (connectionState == CONNECTION_STATE_CLOSED) { return; } @@ -324,7 +324,7 @@ uint32_t Connection::getIP() { return ip; } - std::lock_guard lockClass(connectionLock); + std::scoped_lock lockClass(connectionLock); // IP-address is expressed in network byte order std::error_code error;