improve: use modern implementations to manage threads (#1756)

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

@@ -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,27 +92,26 @@ 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...");
 
 				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;

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<std::mutex> loaderUniqueLock;
-	std::string threadFailMsg;
-
-	bool loaderDone = false;
-	bool loadFailed = false;
+	std::atomic<LoaderStatus> loaderStatus = LoaderStatus::LOADING;
 
 	void logInfos();
 	static void toggleForceCloseButton();

src/creatures/players/management/ban.cpp

@@ -15,7 +15,7 @@
 #include "utils/tools.hpp"
 
 bool Ban::acceptConnection(uint32_t clientIP) {
-	std::lock_guard<std::recursive_mutex> lockClass(lock);
+	std::scoped_lock<std::recursive_mutex> lockClass(lock);
 
 	uint64_t currentTime = OTSYS_TIME();
 

src/game/game.cpp

@@ -9864,7 +9864,7 @@ void Game::playerRewardChestCollect(uint32_t playerId, const Position &pos, uint
 		reward->setParent(playerRewardChest);
 	}
 
-	std::lock_guard<std::mutex> lock(player->quickLootMutex);
+	std::scoped_lock<std::mutex> lock(player->quickLootMutex);
 
 	ReturnValue returnValue = collectRewardChestItems(player, maxMoveItems);
 	if (returnValue != RETURNVALUE_NOERROR) {

src/kv/kv.cpp

@@ -27,7 +27,7 @@ void KVStore::set(const std::string &key, const std::initializer_list<std::pair<
 }
 
 void KVStore::set(const std::string &key, const ValueWrapper &value) {
-	std::lock_guard lock(mutex_);
+	std::scoped_lock lock(mutex_);
 	return setLocked(key, value);
 }
 
@@ -54,7 +54,7 @@ void KVStore::setLocked(const std::string &key, const ValueWrapper &value) {
 
 std::optional<ValueWrapper> 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) {

src/lib/di/shared.hpp

@@ -29,9 +29,9 @@ namespace extension {
 #if !defined(BOOST_DI_NOT_THREAD_SAFE)
 				//<<lock mutex so that move will be synchronized>>
 				explicit scope(scope &&other) noexcept :
-					scope(std::move(other), std::lock_guard<std::mutex>(other.mutex_)) { }
+					scope(std::move(other), std::scoped_lock<std::mutex>(other.mutex_)) { }
 				//<<synchronized move constructor>>
-				scope(scope &&other, const std::lock_guard<std::mutex> &) noexcept :
+				scope(scope &&other, const std::scoped_lock<std::mutex> &) noexcept :
 					object_(std::move(other.object_)) { }
 #endif
 
@@ -49,7 +49,7 @@ namespace extension {
 				wrappers::shared<shared, T> create(const TProvider &provider) & {
 					if (!object_) {
 #if !defined(BOOST_DI_NOT_THREAD_SAFE)
-						std::lock_guard<std::mutex> lock(mutex_);
+						std::scoped_lock<std::mutex> lock(mutex_);
 						if (!object_)
 #endif
 							object_ = std::shared_ptr<T> { provider.get() };
@@ -65,7 +65,7 @@ namespace extension {
 					auto &object = provider.cfg().template data<T>();
 					if (!object) {
 #if !defined(BOOST_DI_NOT_THREAD_SAFE)
-						std::lock_guard<std::mutex> lock(mutex_);
+						std::scoped_lock<std::mutex> lock(mutex_);
 						if (!object)
 #endif
 							object = std::shared_ptr<T> { provider.get() };

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<std::recursive_mutex> lockClass(connectionLock);
+	std::scoped_lock<std::recursive_mutex> 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<std::recursive_mutex> lockClass(connectionLock);
+	std::scoped_lock<std::recursive_mutex> 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<std::recursive_mutex> lockClass(connectionLock);
+	std::scoped_lock<std::recursive_mutex> 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<std::recursive_mutex> lockClass(connectionLock);
+	std::scoped_lock<std::recursive_mutex> lockClass(connectionLock);
 	readTimer.cancel();
 
 	if (error) {
@@ -275,7 +275,7 @@ void Connection::parsePacket(const std::error_code &error) {
 }
 
 void Connection::resumeWork() {
-	std::lock_guard<std::recursive_mutex> lockClass(connectionLock);
+	std::scoped_lock<std::recursive_mutex> 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<std::recursive_mutex> lockClass(connectionLock);
+	std::scoped_lock<std::recursive_mutex> lockClass(connectionLock);
 	if (connectionState == CONNECTION_STATE_CLOSED) {
 		return;
 	}
@@ -324,7 +324,7 @@ uint32_t Connection::getIP() {
 		return ip;
 	}
 
-	std::lock_guard<std::recursive_mutex> lockClass(connectionLock);
+	std::scoped_lock<std::recursive_mutex> lockClass(connectionLock);
 
 	// IP-address is expressed in network byte order
 	std::error_code error;