From f6be14d157ade4c580ff3009a84d68444eb4711a Mon Sep 17 00:00:00 2001 From: Benau Date: Sat, 11 Apr 2020 00:33:29 +0800 Subject: [PATCH] Add proper cleaning of thread for news manager --- src/addons/news_manager.cpp | 50 ++++++++++++------------- src/addons/news_manager.hpp | 12 ++++-- src/states_screens/addons_screen.cpp | 2 + src/states_screens/main_menu_screen.cpp | 2 + src/utils/can_be_deleted.hpp | 17 ++++++--- 5 files changed, 48 insertions(+), 35 deletions(-) diff --git a/src/addons/news_manager.cpp b/src/addons/news_manager.cpp index 251a5d0e9..7fb2220ef 100644 --- a/src/addons/news_manager.cpp +++ b/src/addons/news_manager.cpp @@ -31,6 +31,7 @@ #include "utils/translation.hpp" #include "utils/vs.hpp" +#include #include using namespace Online; @@ -56,6 +57,15 @@ NewsManager::NewsManager() : m_news(std::vector()) // --------------------------------------------------------------------------- NewsManager::~NewsManager() { + // If the download thread doesn't finish on time we detach the thread to + // avoid exception + if (m_download_thread.joinable()) + { + if (!CanBeDeleted::canBeDeletedNow()) + m_download_thread.detach(); + else + m_download_thread.join(); + } } // ~NewsManager // --------------------------------------------------------------------------- @@ -68,6 +78,9 @@ NewsManager::~NewsManager() */ void NewsManager::init(bool force_refresh) { + if (m_download_thread.joinable()) + return; + m_force_refresh = force_refresh; // The rest (which potentially involves downloading m_news_filename) is handled @@ -76,34 +89,21 @@ void NewsManager::init(bool force_refresh) // thread anyway (and the addons menu is disabled as a result). if(UserConfigParams::m_internet_status==RequestManager::IPERM_ALLOWED) { - pthread_attr_t attr; - pthread_attr_init(&attr); - pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); - pthread_t thread_id; - int error = pthread_create(&thread_id, &attr, - &NewsManager::downloadNews, this); - if (error) - { - Log::warn("news", "Could not create thread, error=%d", error); - // In this case just execute the downloading code with this thread - downloadNews(this); - } - pthread_attr_destroy(&attr); + CanBeDeleted::resetCanBeDeleted(); + m_download_thread = std::thread(std::bind( + &NewsManager::downloadNews, this)); } - } //init // --------------------------------------------------------------------------- /** This function submits request which will download the m_news_filename file * if necessary. It is running in its own thread, so we can use blocking * download calls without blocking the GUI. - * \param obj This is 'this' object, passed on during pthread creation. */ -void* NewsManager::downloadNews(void *obj) +void NewsManager::downloadNews() { VS::setThreadName("downloadNews"); - NewsManager *me = (NewsManager*)obj; - me->clearErrorMessage(); + clearErrorMessage(); std::string xml_file = file_manager->getAddonsFile(m_news_filename); // Prevent downloading when .part file created, which is already downloaded @@ -118,7 +118,7 @@ void* NewsManager::downloadNews(void *obj) UserConfigParams::m_news_last_updated +UserConfigParams::m_news_frequency < StkTime::getTimeSinceEpoch() || - me->m_force_refresh || + m_force_refresh || !news_exists ) && UserConfigParams::m_internet_status==RequestManager::IPERM_ALLOWED && !file_manager->fileExists(xml_file_part); @@ -181,7 +181,7 @@ void* NewsManager::downloadNews(void *obj) const char *const curl_error = download_req->getDownloadErrorMessage(); error_message = StringUtils::insertValues(error_message, curl_error); addons_manager->setErrorState(); - me->setErrorMessage(error_message); + setErrorMessage(error_message); Log::error("news", core::stringc(error_message).c_str()); } // hadDownloadError } // hadDownloadError @@ -202,19 +202,17 @@ void* NewsManager::downloadNews(void *obj) if(file_manager->fileExists(xml_file)) { xml = new XMLNode(xml_file); - me->checkRedirect(xml); - me->updateNews(xml, xml_file); + checkRedirect(xml); + updateNews(xml, xml_file); if (addons_manager) - addons_manager->init(xml, me->m_force_refresh); + addons_manager->init(xml, m_force_refresh); delete xml; } // We can't finish stk (esp. delete the file manager) before // this part of the code is reached (since otherwise the file // manager might be access after it was deleted). - me->setCanBeDeleted(); - pthread_exit(NULL); - return 0; // prevent warning + CanBeDeleted::setCanBeDeleted(); } // downloadNews // --------------------------------------------------------------------------- diff --git a/src/addons/news_manager.hpp b/src/addons/news_manager.hpp index 98dbee29d..088a347eb 100644 --- a/src/addons/news_manager.hpp +++ b/src/addons/news_manager.hpp @@ -21,6 +21,7 @@ #ifndef SERVER_ONLY #include +#include #include @@ -100,11 +101,13 @@ private: /** True when all .xml files should be re-downloaded. */ bool m_force_refresh; + std::thread m_download_thread; + void checkRedirect(const XMLNode *xml); void updateNews(const XMLNode *xml, const std::string &filename); bool conditionFulfilled(const std::string &cond); - static void* downloadNews(void *obj); + void downloadNews(); NewsManager(); ~NewsManager(); @@ -145,9 +148,12 @@ public: /** Clears the error message. */ void clearErrorMessage() {m_error_message.setAtomic(""); } // ------------------------------------------------------------------------ + void joinDownloadThreadIfExit() + { + if (CanBeDeleted::canBeDeletedNow() && m_download_thread.joinable()) + m_download_thread.join(); + } }; // NewsManager -extern NewsManager *news_manager; - #endif #endif diff --git a/src/states_screens/addons_screen.cpp b/src/states_screens/addons_screen.cpp index 39fc88082..280000be3 100644 --- a/src/states_screens/addons_screen.cpp +++ b/src/states_screens/addons_screen.cpp @@ -503,6 +503,8 @@ void AddonsScreen::setLastSelected() void AddonsScreen::onUpdate(float dt) { #ifndef SERVER_ONLY + NewsManager::get()->joinDownloadThreadIfExit(); + if (m_reloading) { if(UserConfigParams::m_internet_status!=RequestManager::IPERM_ALLOWED) diff --git a/src/states_screens/main_menu_screen.cpp b/src/states_screens/main_menu_screen.cpp index 664d1a7b6..849cf5908 100644 --- a/src/states_screens/main_menu_screen.cpp +++ b/src/states_screens/main_menu_screen.cpp @@ -183,6 +183,8 @@ void MainMenuScreen::init() void MainMenuScreen::onUpdate(float delta) { #ifndef SERVER_ONLY + NewsManager::get()->joinDownloadThreadIfExit(); + IconButtonWidget* addons_icon = getWidget("addons"); if (addons_icon != NULL) { diff --git a/src/utils/can_be_deleted.hpp b/src/utils/can_be_deleted.hpp index 000d9e698..509015228 100644 --- a/src/utils/can_be_deleted.hpp +++ b/src/utils/can_be_deleted.hpp @@ -20,9 +20,10 @@ #define HEADER_CAN_BE_DELETED #include "utils/log.hpp" -#include "utils/synchronised.hpp" #include "utils/time.hpp" +#include + /** A simple class that a adds a function to wait with a timeout for a * class to be ready to be deleted. It is used for objects with their * own threads (e.g. RequestManager) to make sure they can be deleted. @@ -36,25 +37,29 @@ class CanBeDeleted { private: - Synchronised m_can_be_deleted; + std::atomic_bool m_can_be_deleted; public: /** Set this instance to be not ready to be deleted. */ - CanBeDeleted() { m_can_be_deleted.setAtomic(false); } + CanBeDeleted() { m_can_be_deleted.store(false); } // ------------------------------------------------------------------------ /** Sets this instance to be ready to be deleted. */ - void setCanBeDeleted() {m_can_be_deleted.setAtomic(true); } + void setCanBeDeleted() { m_can_be_deleted.store(true); } + // ------------------------------------------------------------------------ + void resetCanBeDeleted() { m_can_be_deleted.store(false); } + // ------------------------------------------------------------------------ + bool canBeDeletedNow() { return m_can_be_deleted.load(); } // ------------------------------------------------------------------------ /** Waits at most t seconds for this class to be ready to be deleted. * \return true if the class is ready, false in case of a time out. */ bool waitForReadyToDeleted(float waiting_time) { - if (m_can_be_deleted.getAtomic()) return true; + if (m_can_be_deleted.load()) return true; double start = StkTime::getRealTime(); Log::verbose("Thread", "Start waiting %lf", start); while(1) { - if(m_can_be_deleted.getAtomic()) + if(m_can_be_deleted.load()) { Log::verbose("Thread", "Waited %lf seconds for thread to become deleteable.",