refactor: "requestLockerItems" for improved safety and clarity (opent…

…ibiabr#2565)

This introduces a refactoring of the requestLockerItems function to
enhance its safety, prevent potential infinite loops, and improve code
clarity. Key changes include:

• Loop Structure: Replaced the original do-while loop with a safer for loop that ensures all accesses are within vector bounds.
• Loop Removal: Eliminated the second do-while loop that manipulated countSize, which could lead to infinite loops and overflow issues.
• Simplified Item Handling: Streamlined the handling of items from stashToSend, directly updating lockerItems without unnecessary intermediate steps.
• General Cleanup: Improved overall readability and maintainability of
the code, making it easier to understand and less prone to errors.

These modifications ensure that the function behaves as intended while being more robust and easier to debug.

Additionally, a small problem was fixed that prevented the server from shutting down.

Resolves opentibiabr#2561

src/canary_server.cpp

@@ -389,4 +389,5 @@ void CanaryServer::shutdown() {
 	g_dispatcher().shutdown();
 	g_metrics().shutdown();
 	inject<ThreadPool>().shutdown();
+	std::exit(0);
 }

src/creatures/players/player.cpp

@@ -6910,8 +6910,9 @@ std::shared_ptr<Item> Player::getItemFromDepotSearch(uint16_t itemId, const Posi
 	return nullptr;
 }
 
-std::pair<std::vector<std::shared_ptr<Item>>, std::map<uint16_t, std::map<uint8_t, uint32_t>>> Player::requestLockerItems(std::shared_ptr<DepotLocker> depotLocker, bool sendToClient /*= false*/, uint8_t tier /*= 0*/) const {
-	if (depotLocker == nullptr) {
+std::pair<std::vector<std::shared_ptr<Item>>, std::map<uint16_t, std::map<uint8_t, uint32_t>>>
+Player::requestLockerItems(std::shared_ptr<DepotLocker> depotLocker, bool sendToClient /*= false*/, uint8_t tier /*= 0*/) const {
+	if (!depotLocker) {
 		g_logger().error("{} - Depot locker is nullptr", __FUNCTION__);
 		return {};
 	}
@@ -6920,62 +6921,43 @@ std::pair<std::vector<std::shared_ptr<Item>>, std::map<uint16_t, std::map<uint8_
 	std::vector<std::shared_ptr<Item>> itemVector;
 	std::vector<std::shared_ptr<Container>> containers { depotLocker };
 
-	size_t size = 0;
-	do {
-		std::shared_ptr<Container> container = containers[size];
-		size++;
+	for (size_t i = 0; i < containers.size(); ++i) {
+		std::shared_ptr<Container> container = containers[i];
 
-		for (std::shared_ptr<Item> item : container->getItemList()) {
+		for (const auto &item : container->getItemList()) {
 			std::shared_ptr<Container> lockerContainers = item->getContainer();
 			if (lockerContainers && !lockerContainers->empty()) {
 				containers.push_back(lockerContainers);
 				continue;
 			}
 
-			if (item->isStoreItem()) {
-				continue;
-			}
-
 			const ItemType &itemType = Item::items[item->getID()];
-			if (itemType.wareId == 0) {
+			if (item->isStoreItem() || itemType.wareId == 0) {
 				continue;
 			}
 
 			if (lockerContainers && (!itemType.isContainer() || lockerContainers->capacity() != itemType.maxItems)) {
 				continue;
 			}
 
-			if (!item->hasMarketAttributes()) {
-				continue;
-			}
-
-			if (!sendToClient && item->getTier() != tier) {
+			if (!item->hasMarketAttributes() || (!sendToClient && item->getTier() != tier)) {
 				continue;
 			}
 
-			(lockerItems[itemType.wareId])[item->getTier()] += Item::countByType(item, -1);
+			lockerItems[itemType.wareId][item->getTier()] += Item::countByType(item, -1);
 			itemVector.push_back(item);
 		}
-	} while (size < containers.size());
-	StashItemList stashToSend = getStashItems();
-	uint32_t countSize = 0;
-	for (auto [itemId, itemCount] : stashToSend) {
-		countSize += itemCount;
 	}
 
-	do {
-		for (auto [itemId, itemCount] : stashToSend) {
-			const ItemType &itemType = Item::items[itemId];
-			if (itemType.wareId == 0) {
-				continue;
-			}
-
-			countSize = countSize - itemCount;
-			(lockerItems[itemType.wareId])[0] += itemCount;
+	StashItemList stashToSend = getStashItems();
+	for (const auto &[itemId, itemCount] : stashToSend) {
+		const ItemType &itemType = Item::items[itemId];
+		if (itemType.wareId != 0) {
+			lockerItems[itemType.wareId][0] += itemCount;
 		}
-	} while (countSize > 0);
+	}
 
-	return std::make_pair(itemVector, lockerItems);
+	return { itemVector, lockerItems };
 }
 
 std::pair<std::vector<std::shared_ptr<Item>>, uint16_t> Player::getLockerItemsAndCountById(const std::shared_ptr<DepotLocker> &depotLocker, uint8_t tier, uint16_t itemId) {

src/lib/thread/thread_pool.cpp

@@ -8,7 +8,10 @@
  */
 
 #include "pch.hpp"
+
 #include "lib/thread/thread_pool.hpp"
+
+#include "game/game.hpp"
 #include "utils/tools.hpp"
 
 #ifndef DEFAULT_NUMBER_OF_THREADS
@@ -65,7 +68,6 @@ void ThreadPool::shutdown() {
 	while (status == std::future_status::timeout && std::chrono::steady_clock::now() - start < timeout) {
 		tries++;
 		if (tries > 5) {
-			logger.error("Thread pool shutdown timed out.");
 			break;
 		}
 		for (auto &future : futures) {
@@ -84,7 +86,9 @@ asio::io_context &ThreadPool::getIoContext() {
 void ThreadPool::addLoad(const std::function<void(void)> &load) {
 	asio::post(ioService, [this, load]() {
 		if (ioService.stopped()) {
-			logger.error("Shutting down, cannot execute task.");
+			if (g_game().getGameState() != GAME_STATE_SHUTDOWN) {
+				logger.error("Shutting down, cannot execute task.");
+			}
 			return;
 		}