Add proper cleaning of thread for news manager

This commit is contained in:
Benau 2020-04-11 00:33:29 +08:00
parent ccc4bc793d
commit f6be14d157
5 changed files with 48 additions and 35 deletions

View File

@ -31,6 +31,7 @@
#include "utils/translation.hpp" #include "utils/translation.hpp"
#include "utils/vs.hpp" #include "utils/vs.hpp"
#include <functional>
#include <iostream> #include <iostream>
using namespace Online; using namespace Online;
@ -56,6 +57,15 @@ NewsManager::NewsManager() : m_news(std::vector<NewsMessage>())
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
NewsManager::~NewsManager() 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 } // ~NewsManager
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
@ -68,6 +78,9 @@ NewsManager::~NewsManager()
*/ */
void NewsManager::init(bool force_refresh) void NewsManager::init(bool force_refresh)
{ {
if (m_download_thread.joinable())
return;
m_force_refresh = force_refresh; m_force_refresh = force_refresh;
// The rest (which potentially involves downloading m_news_filename) is handled // 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). // thread anyway (and the addons menu is disabled as a result).
if(UserConfigParams::m_internet_status==RequestManager::IPERM_ALLOWED) if(UserConfigParams::m_internet_status==RequestManager::IPERM_ALLOWED)
{ {
pthread_attr_t attr; CanBeDeleted::resetCanBeDeleted();
pthread_attr_init(&attr); m_download_thread = std::thread(std::bind(
pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); &NewsManager::downloadNews, this));
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);
} }
} //init } //init
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
/** This function submits request which will download the m_news_filename file /** 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 * if necessary. It is running in its own thread, so we can use blocking
* download calls without blocking the GUI. * 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"); VS::setThreadName("downloadNews");
NewsManager *me = (NewsManager*)obj; clearErrorMessage();
me->clearErrorMessage();
std::string xml_file = file_manager->getAddonsFile(m_news_filename); std::string xml_file = file_manager->getAddonsFile(m_news_filename);
// Prevent downloading when .part file created, which is already downloaded // 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_last_updated
+UserConfigParams::m_news_frequency +UserConfigParams::m_news_frequency
< StkTime::getTimeSinceEpoch() || < StkTime::getTimeSinceEpoch() ||
me->m_force_refresh || m_force_refresh ||
!news_exists ) !news_exists )
&& UserConfigParams::m_internet_status==RequestManager::IPERM_ALLOWED && UserConfigParams::m_internet_status==RequestManager::IPERM_ALLOWED
&& !file_manager->fileExists(xml_file_part); && !file_manager->fileExists(xml_file_part);
@ -181,7 +181,7 @@ void* NewsManager::downloadNews(void *obj)
const char *const curl_error = download_req->getDownloadErrorMessage(); const char *const curl_error = download_req->getDownloadErrorMessage();
error_message = StringUtils::insertValues(error_message, curl_error); error_message = StringUtils::insertValues(error_message, curl_error);
addons_manager->setErrorState(); addons_manager->setErrorState();
me->setErrorMessage(error_message); setErrorMessage(error_message);
Log::error("news", core::stringc(error_message).c_str()); Log::error("news", core::stringc(error_message).c_str());
} // hadDownloadError } // hadDownloadError
} // hadDownloadError } // hadDownloadError
@ -202,19 +202,17 @@ void* NewsManager::downloadNews(void *obj)
if(file_manager->fileExists(xml_file)) if(file_manager->fileExists(xml_file))
{ {
xml = new XMLNode(xml_file); xml = new XMLNode(xml_file);
me->checkRedirect(xml); checkRedirect(xml);
me->updateNews(xml, xml_file); updateNews(xml, xml_file);
if (addons_manager) if (addons_manager)
addons_manager->init(xml, me->m_force_refresh); addons_manager->init(xml, m_force_refresh);
delete xml; delete xml;
} }
// We can't finish stk (esp. delete the file manager) before // We can't finish stk (esp. delete the file manager) before
// this part of the code is reached (since otherwise the file // this part of the code is reached (since otherwise the file
// manager might be access after it was deleted). // manager might be access after it was deleted).
me->setCanBeDeleted(); CanBeDeleted::setCanBeDeleted();
pthread_exit(NULL);
return 0; // prevent warning
} // downloadNews } // downloadNews
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------

View File

@ -21,6 +21,7 @@
#ifndef SERVER_ONLY #ifndef SERVER_ONLY
#include <string> #include <string>
#include <thread>
#include <vector> #include <vector>
@ -100,11 +101,13 @@ private:
/** True when all .xml files should be re-downloaded. */ /** True when all .xml files should be re-downloaded. */
bool m_force_refresh; bool m_force_refresh;
std::thread m_download_thread;
void checkRedirect(const XMLNode *xml); void checkRedirect(const XMLNode *xml);
void updateNews(const XMLNode *xml, void updateNews(const XMLNode *xml,
const std::string &filename); const std::string &filename);
bool conditionFulfilled(const std::string &cond); bool conditionFulfilled(const std::string &cond);
static void* downloadNews(void *obj); void downloadNews();
NewsManager(); NewsManager();
~NewsManager(); ~NewsManager();
@ -145,9 +148,12 @@ public:
/** Clears the error message. */ /** Clears the error message. */
void clearErrorMessage() {m_error_message.setAtomic(""); } void clearErrorMessage() {m_error_message.setAtomic(""); }
// ------------------------------------------------------------------------ // ------------------------------------------------------------------------
void joinDownloadThreadIfExit()
{
if (CanBeDeleted::canBeDeletedNow() && m_download_thread.joinable())
m_download_thread.join();
}
}; // NewsManager }; // NewsManager
extern NewsManager *news_manager;
#endif #endif
#endif #endif

View File

@ -503,6 +503,8 @@ void AddonsScreen::setLastSelected()
void AddonsScreen::onUpdate(float dt) void AddonsScreen::onUpdate(float dt)
{ {
#ifndef SERVER_ONLY #ifndef SERVER_ONLY
NewsManager::get()->joinDownloadThreadIfExit();
if (m_reloading) if (m_reloading)
{ {
if(UserConfigParams::m_internet_status!=RequestManager::IPERM_ALLOWED) if(UserConfigParams::m_internet_status!=RequestManager::IPERM_ALLOWED)

View File

@ -183,6 +183,8 @@ void MainMenuScreen::init()
void MainMenuScreen::onUpdate(float delta) void MainMenuScreen::onUpdate(float delta)
{ {
#ifndef SERVER_ONLY #ifndef SERVER_ONLY
NewsManager::get()->joinDownloadThreadIfExit();
IconButtonWidget* addons_icon = getWidget<IconButtonWidget>("addons"); IconButtonWidget* addons_icon = getWidget<IconButtonWidget>("addons");
if (addons_icon != NULL) if (addons_icon != NULL)
{ {

View File

@ -20,9 +20,10 @@
#define HEADER_CAN_BE_DELETED #define HEADER_CAN_BE_DELETED
#include "utils/log.hpp" #include "utils/log.hpp"
#include "utils/synchronised.hpp"
#include "utils/time.hpp" #include "utils/time.hpp"
#include <atomic>
/** A simple class that a adds a function to wait with a timeout for a /** 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 * 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. * own threads (e.g. RequestManager) to make sure they can be deleted.
@ -36,25 +37,29 @@
class CanBeDeleted class CanBeDeleted
{ {
private: private:
Synchronised<bool> m_can_be_deleted; std::atomic_bool m_can_be_deleted;
public: public:
/** Set this instance to be not ready to be deleted. */ /** 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. */ /** 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. /** 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. * \return true if the class is ready, false in case of a time out.
*/ */
bool waitForReadyToDeleted(float waiting_time) 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(); double start = StkTime::getRealTime();
Log::verbose("Thread", "Start waiting %lf", start); Log::verbose("Thread", "Start waiting %lf", start);
while(1) while(1)
{ {
if(m_can_be_deleted.getAtomic()) if(m_can_be_deleted.load())
{ {
Log::verbose("Thread", Log::verbose("Thread",
"Waited %lf seconds for thread to become deleteable.", "Waited %lf seconds for thread to become deleteable.",