Merge pull request #12371 from unknownbrackets/thread-detach

Avoid thread.detach(), join when needed instead

Core/FileLoaders/CachingFileLoader.cpp

@@ -109,9 +109,11 @@ void CachingFileLoader::ShutdownCache() {
 	// TODO: Maybe add some hint that deletion is coming soon?
 	// We can't delete while the thread is running, so have to wait.
 	// This should only happen from the menu.
-	while (aheadThread_) {
+	while (aheadThreadRunning_) {
 		sleep_ms(1);
 	}
+	if (aheadThread_.joinable())
+		aheadThread_.join();
 
 	std::lock_guard<std::recursive_mutex> guard(blocksMutex_);
 	for (auto block : blocks_) {
@@ -252,7 +254,7 @@ bool CachingFileLoader::MakeCacheSpaceFor(size_t blocks, bool readingAhead) {
 
 void CachingFileLoader::StartReadAhead(s64 pos) {
 	std::lock_guard<std::recursive_mutex> guard(blocksMutex_);
-	if (aheadThread_) {
+	if (aheadThreadRunning_) {
 		// Already going.
 		return;
 	}
@@ -261,8 +263,10 @@ void CachingFileLoader::StartReadAhead(s64 pos) {
 		return;
 	}
 
-	aheadThread_ = true;
-	std::thread th([this, pos] {
+	aheadThreadRunning_ = true;
+	if (aheadThread_.joinable())
+		aheadThread_.join();
+	aheadThread_ = std::thread([this, pos] {
 		setCurrentThreadName("FileLoaderReadAhead");
 
 		std::unique_lock<std::recursive_mutex> guard(blocksMutex_);
@@ -278,7 +282,6 @@ void CachingFileLoader::StartReadAhead(s64 pos) {
 			}
 		}
 
-		aheadThread_ = false;
+		aheadThreadRunning_ = false;
 	});
-	th.detach();
 }

Core/FileLoaders/CachingFileLoader.h

@@ -19,6 +19,7 @@
 
 #include <map>
 #include <mutex>
+#include <thread>
 
 #include "Common/CommonTypes.h"
 #include "Core/Loaders.h"
@@ -75,6 +76,7 @@ private:
 
 	std::map<s64, BlockInfo> blocks_;
 	std::recursive_mutex blocksMutex_;
-	bool aheadThread_ = false;
+	bool aheadThreadRunning_ = false;
+	std::thread aheadThread_;
 	std::once_flag preparedFlag_;
 };

Core/FileLoaders/RamCachingFileLoader.cpp

@@ -105,9 +105,11 @@ void RamCachingFileLoader::ShutdownCache() {
 
 	// We can't delete while the thread is running, so have to wait.
 	// This should only happen from the menu.
-	while (aheadThread_) {
+	while (aheadThreadRunning_) {
 		sleep_ms(1);
 	}
+	if (aheadThread_.joinable())
+		aheadThread_.join();
 
 	std::lock_guard<std::mutex> guard(blocksMutex_);
 	blocks_.clear();
@@ -118,7 +120,7 @@ void RamCachingFileLoader::ShutdownCache() {
 }
 
 void RamCachingFileLoader::Cancel() {
-	if (aheadThread_) {
+	if (aheadThreadRunning_) {
 		std::lock_guard<std::mutex> guard(blocksMutex_);
 		aheadCancel_ = true;
 	}
@@ -213,14 +215,16 @@ void RamCachingFileLoader::StartReadAhead(s64 pos) {
 
 	std::lock_guard<std::mutex> guard(blocksMutex_);
 	aheadPos_ = pos;
-	if (aheadThread_) {
+	if (aheadThreadRunning_) {
 		// Already going.
 		return;
 	}
 
-	aheadThread_ = true;
+	aheadThreadRunning_ = true;
 	aheadCancel_ = false;
-	std::thread th([this] {
+	if (aheadThread_.joinable())
+		aheadThread_.join();
+	aheadThread_ = std::thread([this] {
 		setCurrentThreadName("FileLoaderReadAhead");
 
 		while (aheadRemaining_ != 0 && !aheadCancel_) {
@@ -243,9 +247,8 @@ void RamCachingFileLoader::StartReadAhead(s64 pos) {
 			}
 		}
 
-		aheadThread_ = false;
+		aheadThreadRunning_ = false;
 	});
-	th.detach();
 }
 
 u32 RamCachingFileLoader::NextAheadBlock() {

Core/FileLoaders/RamCachingFileLoader.h

@@ -19,6 +19,7 @@
 
 #include <vector>
 #include <mutex>
+#include <thread>
 
 #include "Common/CommonTypes.h"
 #include "Core/Loaders.h"
@@ -65,6 +66,7 @@ private:
 	std::mutex blocksMutex_;
 	u32 aheadRemaining_;
 	s64 aheadPos_;
-	bool aheadThread_ = false;
+	std::thread aheadThread_;
+	bool aheadThreadRunning_ = false;
 	bool aheadCancel_ = false;
 };

Core/HLE/sceIo.cpp

@@ -601,7 +601,6 @@ void __IoInit() {
 	if (ioManagerThreadEnabled) {
 		Core_ListenLifecycle(&__IoWakeManager);
 		ioManagerThread = new std::thread(&__IoManagerThread);
-		ioManagerThread->detach();
 	}
 
 	__KernelRegisterWaitTypeFuncs(WAITTYPE_ASYNCIO, __IoAsyncBeginCallback, __IoAsyncEndCallback);
@@ -651,9 +650,10 @@ void __IoShutdown() {
 	ioManagerThreadEnabled = false;
 	ioManager.SyncThread();
 	ioManager.FinishEventLoop();
-	if (ioManagerThread != NULL) {
+	if (ioManagerThread != nullptr) {
+		ioManagerThread->join();
 		delete ioManagerThread;
-		ioManagerThread = NULL;
+		ioManagerThread = nullptr;
 		ioManager.Shutdown();
 	}
 

Core/PSPLoaders.cpp

@@ -57,6 +57,8 @@
 #include "Core/HLE/sceKernelModule.h"
 #include "Core/HLE/sceKernelMemory.h"
 
+static std::thread loadingThread;
+
 static void UseLargeMem(int memsize) {
 	if (memsize != 1) {
 		// Nothing requested.
@@ -256,7 +258,11 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string) {
 	host->SendUIMessage("config_loaded", "");
 	INFO_LOG(LOADER,"Loading %s...", bootpath.c_str());
 
-	std::thread th([bootpath] {
+	PSPLoaders_Shutdown();
+	// Note: this thread reads the game binary, loads caches, and links HLE while UI spins.
+	// To do something deterministically when the game starts, disabling this thread won't be enough.
+	// Instead: Use Core_ListenLifecycle() or watch coreState.
+	loadingThread = std::thread([bootpath] {
 		setCurrentThreadName("ExecLoader");
 		PSP_LoadingLock guard;
 		if (coreState != CORE_POWERUP)
@@ -273,7 +279,6 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string) {
 			PSP_CoreParameter().fileToStart = "";
 		}
 	});
-	th.detach();
 	return true;
 }
 
@@ -379,7 +384,9 @@ bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string) {
 	}
 	// End of temporary code
 
-	std::thread th([finalName] {
+	PSPLoaders_Shutdown();
+	// Note: See Load_PSP_ISO for notes about this thread.
+	loadingThread = std::thread([finalName] {
 		setCurrentThreadName("ExecLoader");
 		PSP_LoadingLock guard;
 		if (coreState != CORE_POWERUP)
@@ -394,7 +401,6 @@ bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string) {
 			PSP_CoreParameter().fileToStart = "";
 		}
 	});
-	th.detach();
 	return true;
 }
 
@@ -402,7 +408,9 @@ bool Load_PSP_GE_Dump(FileLoader *fileLoader, std::string *error_string) {
 	BlobFileSystem *umd = new BlobFileSystem(&pspFileSystem, fileLoader, "data.ppdmp");
 	pspFileSystem.Mount("disc0:", umd);
 
-	std::thread th([] {
+	PSPLoaders_Shutdown();
+	// Note: See Load_PSP_ISO for notes about this thread.
+	loadingThread = std::thread([] {
 		setCurrentThreadName("ExecLoader");
 		PSP_LoadingLock guard;
 		if (coreState != CORE_POWERUP)
@@ -417,6 +425,10 @@ bool Load_PSP_GE_Dump(FileLoader *fileLoader, std::string *error_string) {
 			PSP_CoreParameter().fileToStart = "";
 		}
 	});
-	th.detach();
 	return true;
 }
+
+void PSPLoaders_Shutdown() {
+	if (loadingThread.joinable())
+		loadingThread.join();
+}

Core/PSPLoaders.h

@@ -26,3 +26,4 @@ bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string);
 bool Load_PSP_GE_Dump(FileLoader *fileLoader, std::string *error_string);
 void InitMemoryForGameISO(FileLoader *fileLoader);
 void InitMemoryForGamePBP(FileLoader *fileLoader);
+void PSPLoaders_Shutdown();

Core/Reporting.cpp

@@ -15,6 +15,7 @@
 // Official git repository and contact information can be found at
 // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/.
 
+#include <deque>
 #include <thread>
 #include <mutex>
 #include <condition_variable>
@@ -70,6 +71,13 @@ namespace Reporting
 	// The latest compatibility result from the server.
 	static std::vector<std::string> lastCompatResult;
 
+	static std::mutex pendingMessageLock;
+	static std::condition_variable pendingMessageCond;
+	static std::deque<int> pendingMessages;
+	static bool pendingMessagesDone = false;
+	static std::thread messageThread;
+	static std::thread compatThread;
+
 	enum class RequestType
 	{
 		NONE,
@@ -93,6 +101,7 @@ namespace Reporting
 	static std::condition_variable crcCond;
 	static std::string crcFilename;
 	static std::map<std::string, u32> crcResults;
+	static std::thread crcThread;
 
 	static int CalculateCRCThread() {
 		setCurrentThreadName("ReportCRC");
@@ -133,8 +142,7 @@ namespace Reporting
 		}
 
 		crcFilename = gamePath;
-		std::thread th(CalculateCRCThread);
-		th.detach();
+		crcThread = std::thread(CalculateCRCThread);
 	}
 
 	u32 RetrieveCRC() {
@@ -148,6 +156,8 @@ namespace Reporting
 			it = crcResults.find(gamePath);
 		}
 
+		if (crcThread.joinable())
+			crcThread.join();
 		return it->second;
 	}
 
@@ -297,10 +307,20 @@ namespace Reporting
 		logOnceUsed.clear();
 		everUnsupported = false;
 		currentSupported = IsSupported();
+		pendingMessagesDone = false;
 	}
 
 	void Shutdown()
 	{
+		pendingMessageLock.lock();
+		pendingMessagesDone = true;
+		pendingMessageCond.notify_one();
+		pendingMessageLock.unlock();
+		if (compatThread.joinable())
+			compatThread.join();
+		if (messageThread.joinable())
+			messageThread.join();
+
 		// Just so it can be enabled in the menu again.
 		Init();
 	}
@@ -543,6 +563,28 @@ namespace Reporting
 		return -1;
 	}
 
+	int ProcessPending() {
+		setCurrentThreadName("Report");
+
+		std::unique_lock<std::mutex> guard(pendingMessageLock);
+		while (!pendingMessagesDone) {
+			while (!pendingMessages.empty() && !pendingMessagesDone) {
+				int pos = pendingMessages.front();
+				pendingMessages.pop_front();
+
+				guard.unlock();
+				Process(pos);
+				guard.lock();
+			}
+			if (pendingMessagesDone) {
+				break;
+			}
+			pendingMessageCond.wait(guard);
+		}
+
+		return 0;
+	}
+
 	void ReportMessage(const char *message, ...)
 	{
 		if (!IsEnabled() || CheckSpamLimited())
@@ -565,8 +607,13 @@ namespace Reporting
 		payload.string1 = message;
 		payload.string2 = temp;
 
-		std::thread th(Process, pos);
-		th.detach();
+		std::lock_guard<std::mutex> guard(pendingMessageLock);
+		pendingMessages.push_back(pos);
+		pendingMessageCond.notify_one();
+
+		if (!messageThread.joinable()) {
+			messageThread = std::thread(ProcessPending);
+		}
 	}
 
 	void ReportMessageFormatted(const char *message, const char *formatted)
@@ -582,8 +629,13 @@ namespace Reporting
 		payload.string1 = message;
 		payload.string2 = formatted;
 
-		std::thread th(Process, pos);
-		th.detach();
+		std::lock_guard<std::mutex> guard(pendingMessageLock);
+		pendingMessages.push_back(pos);
+		pendingMessageCond.notify_one();
+
+		if (!messageThread.joinable()) {
+			messageThread = std::thread(ProcessPending);
+		}
 	}
 
 	void ReportCompatibility(const char *compat, int graphics, int speed, int gameplay, const std::string &screenshotFilename)
@@ -602,8 +654,9 @@ namespace Reporting
 		payload.int2 = speed;
 		payload.int3 = gameplay;
 
-		std::thread th(Process, pos);
-		th.detach();
+		if (compatThread.joinable())
+			compatThread.join();
+		compatThread = std::thread(Process, pos);
 	}
 
 	std::vector<std::string> CompatibilitySuggestions() {

Core/SaveState.cpp

@@ -152,11 +152,12 @@ namespace SaveState
 
 		void ScheduleCompress(std::vector<u8> *result, const std::vector<u8> *state, const std::vector<u8> *base)
 		{
-			auto th = new std::thread([=]{
+			if (compressThread_.joinable())
+				compressThread_.join();
+			compressThread_ = std::thread([=]{
 				setCurrentThreadName("SaveStateCompress");
 				Compress(*result, *state, *base);
 			});
-			th->detach();
 		}
 
 		void Compress(std::vector<u8> &result, const std::vector<u8> &state, const std::vector<u8> &base)
@@ -207,6 +208,9 @@ namespace SaveState
 
 		void Clear()
 		{
+			if (compressThread_.joinable())
+				compressThread_.join();
+
 			// This lock is mainly for shutdown.
 			std::lock_guard<std::mutex> guard(lock_);
 			first_ = 0;
@@ -232,6 +236,7 @@ namespace SaveState
 		StateBuffer bases_[2];
 		std::vector<int> baseMapping_;
 		std::mutex lock_;
+		std::thread compressThread_;
 
 		int base_;
 		int baseUsage_;

Core/System.cpp

@@ -268,6 +268,7 @@ PSP_LoadingLock::~PSP_LoadingLock() {
 void CPU_Shutdown() {
 	// Since we load on a background thread, wait for startup to complete.
 	PSP_LoadingLock lock;
+	PSPLoaders_Shutdown();
 
 	if (g_Config.bAutoSaveSymbolMap) {
 		host->SaveSymbolMap();

Core/Util/GameManager.cpp

@@ -44,8 +44,7 @@
 
 GameManager g_GameManager;
 
-GameManager::GameManager()
-	: installInProgress_(false), installProgress_(0.0f) {
+GameManager::GameManager() {
 }
 
 std::string GameManager::GetTempFilename() const {
@@ -66,7 +65,7 @@ bool GameManager::IsGameInstalled(std::string name) {
 }
 
 bool GameManager::DownloadAndInstall(std::string storeFileUrl) {
-	if (curDownload_.get() != 0) {
+	if (curDownload_.get() != nullptr) {
 		ERROR_LOG(HLE, "Can only process one download at a time");
 		return false;
 	}
@@ -131,6 +130,15 @@ void GameManager::Update() {
 		}
 		curDownload_.reset();
 	}
+
+	if (installDonePending_) {
+		if (installThread_.get() != nullptr) {
+			if (installThread_->joinable())
+				installThread_->join();
+			installThread_.reset();
+		}
+		installDonePending_ = false;
+	}
 }
 
 static void countSlashes(const std::string &fileName, int *slashLocation, int *slashCount) {
@@ -608,7 +616,6 @@ bool GameManager::InstallGameOnThread(std::string url, std::string fileName, boo
 		return false;
 	}
 	installThread_.reset(new std::thread(std::bind(&GameManager::InstallGame, this, url, fileName, deleteAfter)));
-	installThread_->detach();
 	return true;
 }
 
@@ -628,7 +635,5 @@ bool GameManager::InstallRawISO(const std::string &file, const std::string &orig
 }
 
 void GameManager::InstallDone() {
-	if (installThread_.get() != 0) {
-		installThread_.reset();
-	}
+	installDonePending_ = true;
 }

Core/Util/GameManager.h

@@ -86,8 +86,9 @@ private:
 	std::string GetISOGameID(FileLoader *loader) const;
 	std::shared_ptr<http::Download> curDownload_;
 	std::shared_ptr<std::thread> installThread_;
-	bool installInProgress_;
-	float installProgress_;
+	bool installInProgress_ = false;
+	bool installDonePending_ = false;
+	float installProgress_ = 0.0f;
 	std::string installError_;
 };
 

Core/WebServer.cpp

@@ -288,7 +288,6 @@ bool StartWebServer(WebServerFlags flags) {
 		serverStatus = ServerStatus::STARTING;
 		serverFlags = (int)flags;
 		serverThread = std::thread(&ExecuteWebServer);
-		serverThread.detach();
 		return true;
 
 	default:
@@ -321,3 +320,9 @@ bool WebServerStopped(WebServerFlags flags) {
 	}
 	return serverStatus == ServerStatus::STOPPED;
 }
+
+void ShutdownWebServer() {
+	StopWebServer(WebServerFlags::ALL);
+	if (serverThread.joinable())
+		serverThread.join();
+}

Core/WebServer.h

@@ -26,5 +26,6 @@ bool StartWebServer(WebServerFlags flags);
 bool StopWebServer(WebServerFlags flags);
 bool WebServerStopping(WebServerFlags flags);
 bool WebServerStopped(WebServerFlags flags);
+void ShutdownWebServer();
 
 bool RemoteISOFileSupported(const std::string &filename);

UI/NativeApp.cpp

@@ -879,11 +879,12 @@ void NativeShutdownGraphics() {
 	winAudioBackend = nullptr;
 #endif
 
+	ShutdownWebServer();
+	UIBackgroundShutdown();
+
 	delete g_gameInfoCache;
 	g_gameInfoCache = nullptr;
 
-	UIBackgroundShutdown();
-
 	delete uiContext;
 	uiContext = nullptr;
 

UI/RemoteISOScreen.cpp

@@ -257,7 +257,6 @@ RemoteISOConnectScreen::RemoteISOConnectScreen() : status_(ScanStatus::SCANNING)
 	scanThread_ = new std::thread([](RemoteISOConnectScreen *thiz) {
 		thiz->ExecuteScan();
 	}, this);
-	scanThread_->detach();
 }
 
 RemoteISOConnectScreen::~RemoteISOConnectScreen() {
@@ -271,6 +270,8 @@ RemoteISOConnectScreen::~RemoteISOConnectScreen() {
 			break;
 		}
 	}
+	if (scanThread_->joinable())
+		scanThread_->join();
 	delete scanThread_;
 }
 
@@ -314,11 +315,12 @@ void RemoteISOConnectScreen::update() {
 		status_ = ScanStatus::LOADING;
 
 		// Let's reuse scanThread_.
+		if (scanThread_->joinable())
+			scanThread_->join();
 		delete scanThread_;
 		scanThread_ = new std::thread([](RemoteISOConnectScreen *thiz) {
 			thiz->ExecuteLoad();
 		}, this);
-		scanThread_->detach();
 		break;
 
 	case ScanStatus::FAILED:
@@ -331,11 +333,12 @@ void RemoteISOConnectScreen::update() {
 			status_ = ScanStatus::SCANNING;
 			nextRetry_ = 0.0;
 
+			if (scanThread_->joinable())
+				scanThread_->join();
 			delete scanThread_;
 			scanThread_ = new std::thread([](RemoteISOConnectScreen *thiz) {
 				thiz->ExecuteScan();
 			}, this);
-			scanThread_->detach();
 		}
 		break;
 

ext/native/net/http_client.cpp

@@ -427,16 +427,16 @@ int Client::ReadResponseEntity(Buffer *readbuf, const std::vector<std::string> &
 }
 
 Download::Download(const std::string &url, const std::string &outfile)
-	: progress_(0.0f), url_(url), outfile_(outfile), resultCode_(0), completed_(false), failed_(false), cancelled_(false), hidden_(false) {
+	: url_(url), outfile_(outfile) {
 }
 
 Download::~Download() {
-
+	if (thread_.joinable())
+		thread_.join();
 }
 
 void Download::Start(std::shared_ptr<Download> self) {
-	std::thread th(std::bind(&Download::Do, this, self));
-	th.detach();
+	thread_ = std::thread(std::bind(&Download::Do, this, self));
 }
 
 void Download::SetFailed(int code) {
@@ -576,6 +576,7 @@ void Downloader::CancelAll() {
 	for (size_t i = 0; i < downloads_.size(); i++) {
 		downloads_[i]->Cancel();
 	}
+	downloads_.clear();
 }
 
 }	// http

ext/native/net/http_client.h

@@ -142,16 +142,17 @@ private:
 	int PerformGET(const std::string &url);
 	std::string RedirectLocation(const std::string &baseUrl);
 	void SetFailed(int code);
-	float progress_;
+	float progress_ = 0.0f;
 	Buffer buffer_;
 	std::vector<std::string> responseHeaders_;
 	std::string url_;
 	std::string outfile_;
-	int resultCode_;
-	bool completed_;
-	bool failed_;
-	bool cancelled_;
-	bool hidden_;
+	std::thread thread_;
+	int resultCode_ = 0;
+	bool completed_ = false;
+	bool failed_ = false;
+	bool cancelled_ = false;
+	bool hidden_ = false;
 	std::function<void(Download &)> callback_;
 };